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). Okay. Some comments below. > Migrate Obj-C++ part of redecl-merge into separate test, now that this is > growing. > templates.mm: test that CodeGen has seen exactly one definition of template > instantiations. > redecl-merge.m: use "@" specifier for expected-diagnostics. Thanks for migrating this. > Added: > cfe/trunk/test/Modules/Inputs/templates-left.h (with props) > cfe/trunk/test/Modules/Inputs/templates-right.h (with props) > cfe/trunk/test/Modules/Inputs/templates-top.h (with props) > cfe/trunk/test/Modules/templates.mm > Modified: > cfe/trunk/include/clang/Serialization/ASTReader.h > cfe/trunk/lib/Serialization/ASTReader.cpp > cfe/trunk/lib/Serialization/ASTReaderDecl.cpp > cfe/trunk/test/Modules/Inputs/module.map > cfe/trunk/test/Modules/Inputs/redecl-merge-bottom.h > cfe/trunk/test/Modules/Inputs/redecl-merge-left.h > cfe/trunk/test/Modules/Inputs/redecl-merge-right.h > cfe/trunk/test/Modules/Inputs/redecl-merge-top.h > cfe/trunk/test/Modules/redecl-merge.m > > Modified: cfe/trunk/include/clang/Serialization/ASTReader.h > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Serialization/ASTReader.h?rev=164993&r1=164992&r2=164993&view=diff > ============================================================================== > --- cfe/trunk/include/clang/Serialization/ASTReader.h (original) > +++ cfe/trunk/include/clang/Serialization/ASTReader.h Tue Oct 2 04:09:43 2012 > @@ -687,7 +687,7 @@ > /// Objective-C protocols. > std::deque<Decl *> InterestingDecls; > > - /// \brief The set of redeclarable declaraations that have been > deserialized > + /// \brief The set of redeclarable declarations that have been deserialized > /// since the last time the declaration chains were linked. > llvm::SmallPtrSet<Decl *, 16> RedeclsDeserialized; > > @@ -854,6 +854,10 @@ > > void finishPendingActions(); > > + /// \brief Whether D needs to be instantiated, i.e. whether an > instantiation > + /// for D does not exist yet. > + bool needPendingInstantiation(ValueDecl* D) const; > + > /// \brief Produce an error diagnostic and return true. > /// > /// This routine should only be used for fatal errors that have to > > 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. > Modified: cfe/trunk/lib/Serialization/ASTReaderDecl.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTReaderDecl.cpp?rev=164993&r1=164992&r2=164993&view=diff > ============================================================================== > --- cfe/trunk/lib/Serialization/ASTReaderDecl.cpp (original) > +++ cfe/trunk/lib/Serialization/ASTReaderDecl.cpp Tue Oct 2 04:09:43 2012 > @@ -2156,7 +2156,7 @@ > // loading, and some declarations may still be initializing. > if (isConsumerInterestedIn(D)) > InterestingDecls.push_back(D); > - > + > return D; > } > > @@ -2504,3 +2504,60 @@ > } > } > } > + > +/// \brief Return a template specialization of ND (should be a TemplateDecl) > +/// that matches FD or TD. > +static NamedDecl* findMatchingSpecialization(FunctionDecl* FD, > + > ClassTemplateSpecializationDecl*TD, > + NamedDecl* ND) { > + TemplateDecl* Templt = dyn_cast<TemplateDecl>(ND); > + if (!Templt) return 0; > + if (FD) { > + FunctionTemplateDecl* FTD = dyn_cast<FunctionTemplateDecl>(Templt); > + if (!FTD) return 0; > + const TemplateArgumentList* TmpltArgs = > FD->getTemplateSpecializationArgs(); > + assert(TmpltArgs || "Template without arguments"); > + void* InsertionPoint; > + return FTD->findSpecialization(TmpltArgs->data(), TmpltArgs->size(), > + InsertionPoint); Function templates can be overloaded, and you're not checking that FD is actually a specialization of FTD, so it looks like this can confuse instantiations of different templates that happen to have the same template arguments, e.g., f<int> for template<typename T> void f(T); template<typename T> void f(T*); > + } else { > + ClassTemplateDecl* CTD = dyn_cast<ClassTemplateDecl>(Templt); > + if (!CTD) return 0; > + const TemplateArgumentList& TmpltArgs = TD->getTemplateArgs(); > + void* InsertionPoint; > + return CTD->findSpecialization(TmpltArgs.data(), TmpltArgs.size(), > + InsertionPoint); > + } > + return 0; > +} > + > +/// \brief Find out whether an instantiation (outside the module) already > exists > +bool ASTReader::needPendingInstantiation(ValueDecl* D) const { > + DeclContext *DC = D->getDeclContext()->getRedeclContext(); > + DeclarationName Name = D->getDeclName(); > + assert(Name && "unnamed template"); > + > + FunctionDecl* FD = dyn_cast<FunctionDecl>(D); > + ClassTemplateSpecializationDecl* CD > + = FD ? 0 : dyn_cast<ClassTemplateSpecializationDecl>(D); > + > + NamedDecl* FoundSpecialization = 0; > + if (DC->isTranslationUnit() && SemaObj) { > + IdentifierResolver &IdResolver = SemaObj->IdResolver; > + for (IdentifierResolver::iterator I = IdResolver.begin(Name), > + IEnd = IdResolver.end(); > + I != IEnd && !FoundSpecialization; ++I) > + FoundSpecialization = findMatchingSpecialization(FD, CD, *I); > + } else { > + // templates are redeclarables, i.e. they must have been merged into > + // the primary context. Use localUncachedLookup to not pick up template > + // decls from modules again. > + llvm::SmallVector<NamedDecl*, 6> Results; > + DC->getPrimaryContext()->localUncachedLookup(Name, Results); > + for (llvm::SmallVector<NamedDecl *, 6>::const_iterator > + I = Results.begin(), E = Results.end(); > + I != E && FoundSpecialization; ++I) > + FoundSpecialization = findMatchingSpecialization(FD, CD, *I); > + } > + return FoundSpecialization && isSameEntity(FoundSpecialization, D); > +} Why this this walking through trying to match up names? If D is fact an instantiation, we can find the template (or member of a template) from which it was instantiated directly, rather than searching for it. - Doug _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
