[PATCH] D12205: LTO: Simplify merged module ownership.

[Peter Collingbourne via llvm-commits]

On Fri, Aug 21, 2015 at 05:48:10PM -0700, Duncan P. N. Exon Smith wrote:
> > Index: lib/LTO/LTOCodeGenerator.cpp
> > ===================================================================
> > --- lib/LTO/LTOCodeGenerator.cpp
> > +++ lib/LTO/LTOCodeGenerator.cpp
> > @@ -64,29 +64,20 @@
> >  }
> >  
> >  LTOCodeGenerator::LTOCodeGenerator()
> > -    : Context(getGlobalContext()), IRLinker(new Module("ld-temp.o", Context)) {
> > +    : Context(getGlobalContext()),
> > +      OwnedModule(new Module("ld-temp.o", Context)),
> 
> "OwnedContext" made sense as a name because the context isn't always
> owned.  We access the context through `Context`, but rely on the
> destructor in `OwnedContext` to clean up memory.
> 
> Here, we *always* own the module, so we don't need to distinguish.  I
> think a clearer name might be "MergedModule", a correctly-capitalized
> version of the local variable name used in a few places.

Done in new patch.

> > +      IRLinker(OwnedModule.get()) {
> >    initializeLTOPasses();
> >  }
> >  
> >  LTOCodeGenerator::LTOCodeGenerator(std::unique_ptr<LLVMContext> Context)
> >      : OwnedContext(std::move(Context)), Context(*OwnedContext),
> > -      IRLinker(new Module("ld-temp.o", *OwnedContext)) {
> > +      OwnedModule(new Module("ld-temp.o", *OwnedContext)),
> > +      IRLinker(OwnedModule.get()) {
> >    initializeLTOPasses();
> >  }
> >  
> > -void LTOCodeGenerator::destroyMergedModule() {
> > -  if (OwnedModule) {
> > -    assert(IRLinker.getModule() == &OwnedModule->getModule() &&
> > -           "The linker's module should be the same as the owned module");
> > -    delete OwnedModule;
> > -    OwnedModule = nullptr;
> > -  } else if (IRLinker.getModule())
> > -    IRLinker.deleteModule();
> > -}
> > -
> > -LTOCodeGenerator::~LTOCodeGenerator() {
> > -  destroyMergedModule();
> > -}
> > +LTOCodeGenerator::~LTOCodeGenerator() {}
> >  
> >  // Initialize LTO passes. Please keep this funciton in sync with
> >  // PassManagerBuilder::populateLTOPassManager(), and make sure all LTO
> > @@ -408,7 +397,6 @@
> >  void LTOCodeGenerator::applyScopeRestrictions() {
> >    if (ScopeRestrictionsDone || !ShouldInternalize)
> >      return;
> > -  Module *mergedModule = IRLinker.getModule();
> 
> If you rename this variable in a separate NFC commit to 'MergedModule'
> it would minimize noise in the semantic change.

r245874, this also renames elsewhere and converts the loops to range-for.

Thanks,
-- 
Peter