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

[Duncan P. N. Exon Smith via llvm-commits]

> On 2015-Aug-24, at 14:32, Peter Collingbourne <peter at pcc.me.uk> wrote:
> 
> 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.
> 

Latest patch LGTM (and I've checked with ld64 as well).  Thanks!

>>> +      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