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