[cfe-commits] r164993 - in /cfe/trunk: include/clang/Serialization/ASTReader.h lib/Serialization/ASTReader.cpp lib/Serialization/ASTReaderDecl.cpp test/Modules/Inputs/module.map test/Modules/Inputs/redecl-merge-bottom.h test/Modules/Inputs/redecl-merge-left.h test/Modules/Inputs/redecl-merge-right.h test/Modules/Inputs/redecl-merge-top.h test/Modules/Inputs/templates-left.h test/Modules/Inputs/templates-right.h test/Modules/Inputs/templates-top.h test/Modules/redecl-merge.m test/Modules/templates.mm

[Douglas Gregor]

On Oct 3, 2012, at 11:16 AM, Douglas Gregor <dgregor at apple.com> wrote:

> 
> On Oct 2, 2012, at 2:09 AM, Axel Naumann <Axel.Naumann at cern.ch> wrote:
> 
>> Author: axel
>> Date: Tue Oct  2 04:09:43 2012
>> New Revision: 164993
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=164993&view=rev
>> Log:
>> Merge pending instantiations instead of overwriting existing ones.
>> Check whether a pending instantiation needs to be instantiated (or whether an instantiation already exists).
>> Verify the size of the PendingInstantiations record (was only checking size of existing PendingInstantiations).
>> [snip]
>> Modified: cfe/trunk/lib/Serialization/ASTReader.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTReader.cpp?rev=164993&r1=164992&r2=164993&view=diff
>> ==============================================================================
>> --- cfe/trunk/lib/Serialization/ASTReader.cpp (original)
>> +++ cfe/trunk/lib/Serialization/ASTReader.cpp Tue Oct  2 04:09:43 2012
>> @@ -2223,13 +2223,15 @@
>> 
>>    case PENDING_IMPLICIT_INSTANTIATIONS:
>>      if (PendingInstantiations.size() % 2 != 0) {
>> +        Error("Invalid existing PendingInstantiations");
>> +        return Failure;
>> +      }
>> +
>> +      if (Record.size() % 2 != 0) {
>>        Error("Invalid PENDING_IMPLICIT_INSTANTIATIONS block");
>>        return Failure;
>>      }
>> -        
>> -      // Later lists of pending instantiations overwrite earlier ones.
>> -      // FIXME: This is most certainly wrong for modules.
>> -      PendingInstantiations.clear();
>> +
>>      for (unsigned I = 0, N = Record.size(); I != N; /* in loop */) {
>>        PendingInstantiations.push_back(getGlobalDeclID(F, Record[I++]));
>>        PendingInstantiations.push_back(
>> @@ -5592,7 +5594,11 @@
>>    ValueDecl *D = cast<ValueDecl>(GetDecl(PendingInstantiations[Idx++]));
>>    SourceLocation Loc
>>      = SourceLocation::getFromRawEncoding(PendingInstantiations[Idx++]);
>> -    Pending.push_back(std::make_pair(D, Loc));
>> +
>> +    // For modules, find out whether an instantiation already exists
>> +    if (!getContext().getLangOpts().Modules
>> +        || needPendingInstantiation(D))
>> +      Pending.push_back(std::make_pair(D, Loc));
>>  }  
>>  PendingInstantiations.clear();
>> }
> 
> Why do we even bother with needPendingInstantiation()? Extra pending instantiations don't actually do any harm, because the instantiation routines that Sema::PerformPendingInstantiations delegates to---InstantiateFunctionDefinition and InstantiateStaticDataMemberDefinition---already know how to return early if the thing they are supposed to instantiate already has a definition. So we're doing a bunch of work up front in the ASTReader to save us from a cheap check in Sema.
> 
> FWIW, all tests pass if needPendingInstantiation(D) here always returns true. If you have other code that the call to needPendingInstantiation() fixes, it's a symptom of a different problem.

I ended up reverting the needPendingInstantiation() in r165139. For future non-trivial commits that affect modules, please send a patch rather than relying on post-commit review. Modules are a fairly complicated feature with a lot of interactions with the AST and Sema, and we need to be sure we're building things correctly.

	- Doug
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20121003/98371d44/attachment.html>