[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

[Axel Naumann]

Hi Doug,

Thanks for your reviews + fixes.

On 10/03/2012 08:54 PM, Douglas Gregor wrote:
>> On Oct 2, 2012, at 2:09 AM, Axel Naumann <Axel.Naumann at cern.ch
>> <mailto: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();
>>> }

> I ended up reverting the needPendingInstantiation() in r165139.
> 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.

That's the lesson I learned already from your reviews :-)

> For
> future non-trivial commits that affect modules, please send a patch
> rather than relying on post-commit review.

Will do!

Axel.