On Sep 9, 2011, at 9:12 AM, Douglas Gregor wrote: > > On Sep 9, 2011, at 9:05 AM, Argyrios Kyrtzidis wrote: > >> On Sep 9, 2011, at 8:15 AM, Douglas Gregor wrote: >> >>> >>> On Sep 8, 2011, at 11:44 PM, Argyrios Kyrtzidis wrote: >>> >>>> Author: akirtzidis >>>> Date: Fri Sep 9 01:44:17 2011 >>>> New Revision: 139358 >>>> >>>> URL: http://llvm.org/viewvc/llvm-project?rev=139358&view=rev >>>> Log: >>>> [PCH] When loading the decls linked to an identifier, also make them >>>> visible >>>> in the translation unit. >>>> >>>> Modified: >>>> cfe/trunk/lib/Serialization/ASTReader.cpp >>>> >>>> Modified: cfe/trunk/lib/Serialization/ASTReader.cpp >>>> URL: >>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTReader.cpp?rev=139358&r1=139357&r2=139358&view=diff >>>> ============================================================================== >>>> --- cfe/trunk/lib/Serialization/ASTReader.cpp (original) >>>> +++ cfe/trunk/lib/Serialization/ASTReader.cpp Fri Sep 9 01:44:17 2011 >>>> @@ -4841,8 +4841,16 @@ >>>> return; >>>> } >>>> >>>> + ASTContext &Ctx = *getContext(); >>>> for (unsigned I = 0, N = DeclIDs.size(); I != N; ++I) { >>>> NamedDecl *D = cast<NamedDecl>(GetDecl(DeclIDs[I])); >>>> + >>>> + // In C++, translation unit's visible decls map gets emitted in the >>>> AST >>>> + // file, but not on C; make the decl visible so it can be looked up. >>>> + if (!Ctx.getLangOptions().CPlusPlus) >>>> + SetExternalVisibleDeclsForName(Ctx.getTranslationUnitDecl(), >>>> + DeclarationName(II), D); >>>> + >>>> >>> >>> This is a bit unfortunate. Creating the lookup table in the translation >>> unit forces the AST to continually maintain that lookup table (e.g., adding >>> entries to it every time a visible declaration is added to the translation >>> unit), which is a cost we'd like to avoid in C. I presume that you only >>> made this change to support the new lookup used in r139359? I have an >>> alternative suggestion for that... >> >> But adding entries always updated the lookup table, or the lookup in r139359 >> would not work in non-PCH mode, right ? >> In general I'd find it bad for the AST usefulness if you could not do a >> lookup in translation unit unless it is on C++. > > > It's more subtle than that. The lookup table for a DeclContext is constructed > lazily, when one performs the first lookup, by walking the declarations > lexically in that context and populating the table. Once a table is there, it > is maintained as declarations are added to the DeclContext.
I see the error of my ways now. Note however that, outside of this fix, it's very unfortunate that lookup does not work on a loaded C AST. > > A call to SetExternalVisibleDeclsForName creates the lookup table, so we end > up paying the cost of maintaining the declarations in the DeclContext even if > we never want to perform a lookup in the context. > > - Doug _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
