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 <[email protected] >> <mailto:[email protected]>> 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. _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
