[cfe-commits] r139358 - /cfe/trunk/lib/Serialization/ASTReader.cpp

[Douglas Gregor]

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.

	- Doug