On Sep 9, 2011, at 10:30 AM, Douglas Gregor wrote: > > On Sep 9, 2011, at 10:28 AM, Argyrios Kyrtzidis wrote: > >> On Sep 9, 2011, at 9:55 AM, Douglas Gregor wrote: >> >>> >>> On Sep 9, 2011, at 9:44 AM, Argyrios Kyrtzidis wrote: >>> >>>> >>>> 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. >>> >>> >>> I think it should work, but it's super-inefficient because it will >>> deserialize the whole translation unit. >> >> Once a declaration is loaded through the identifier it can be found in >> lexical decls linked list right ? It's only of matter then to figure out >> that if no result is returned from looking up at the external ast source, it >> doesn't mean that no decl with that name exists, but that the source didn't >> store a map; in which case we recreate the map with only the lexical decls >> that were already loaded (and subsequent loaded-through-identifier decls >> will get added to the existing map) > > > Ahhh, you are correct. Which means this is already broken :( > > Perhaps we should just take the performance hit when building an AST file for > a C translation unit, generating the lookup table for the translation unit as > AST writing time so we can serialize it.
Rough estimate: ObjC PCH for Cocoa.h is -20% compared to the ObjC++ PCH, is this acceptable hit ? > > - Doug _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
