[cfe-commits] r142634 - in /cfe/trunk: include/clang/AST/DeclBase.h lib/AST/ASTImporter.cpp lib/AST/DeclBase.cpp

[Douglas Gregor]

On Oct 24, 2011, at 6:49 PM, Argyrios Kyrtzidis wrote:

> On Oct 20, 2011, at 7:57 PM, Sean Callanan wrote:
> 
>> Author: spyffe
>> Date: Thu Oct 20 21:57:43 2011
>> New Revision: 142634
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=142634&view=rev
>> Log:
>> I added a new function to DeclContext called
>> addDeclInternal().  This function suppresses any
>> calls to FindExternalVisibleDeclsByName() while
>> a Decl is added to a DeclContext.  This behavior
>> is required for the ASTImporter, because in the
>> case of the LLDB client the ASTImporter would be
>> called recursively to import the visible decls,
>> which leads to assertions because the recursive
>> call is seeing partially-formed types.
>> 
>> I also modified the ASTImporter to use
>> addDeclInternal() in all places where it would
>> otherwise use addDecl().  This fix should not
>> affect the rest of Clang, passes Clang's
>> testsuite, and fixes several serious LLDB bugs.
> 
> Could we go in another direction here, making ASTImporter simpler instead of complicating DeclContext ?
> 
> How about, the ASTImporter does not modify DeclContexts at all (maybe if 'MinimalImport' is true) in which case it turns into a simpler "Decl-creation-factory".
> The clients would be responsible to do the right thing with the Decls that they get from ASTImporter, which would either mean adding them to a DeclContext, or, most of the cases involving lldb, returning them via the 'ExternaASTSource' so the DeclContext that already called into ExternaASTSource can add them itself.
> 
> In general, this will get lldb somewhat "closer" to how PCH works instead of diverging even more.

PCH knows the full structure of AST nodes, so it knows when we need to pull in some other nodes (recursively) to be able to describe the nodes it was asked to produce. LLDB shouldn't have to know that information, so the recursion has to happen somewhere. Can we handle the recursion on ASTImporter while still leaving LLDB in charge of adding nodes to the DeclContext? Perhaps, but that feels like an ever more complicated protocol.

	- Doug