On Oct 3, 2012, at 11:16 AM, Douglas Gregor <[email protected]> wrote:
> > On Oct 2, 2012, at 2:09 AM, Axel Naumann <[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(); >> } > > 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
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
