[Lldb-commits] [PATCH] D12658: Search variables based on clang::DeclContext and clang::Decl tree

[Greg Clayton via lldb-commits]

clayborg added a comment.

In http://reviews.llvm.org/D12658#243863, @paulherman wrote:

> I agree with most of your comments and will fix them.
>
> One thing I noticed is that Block::GetDeclContext() does not do the right thing (see inline comment).


Yep, we only ever needed the function decl context before this. We can rename Block::GetDeclContext() to be Block::GetFunctionDeclContext(), and fix all places that use it to use the new name. Then we modify Block::GetDeclContext() to do what you want it to do.

Also note that you don't want any methods on lldb_private classes to be named "GetClangXXX()" since these objects are abstracted from specific type systems, everything has to be generic and abstract.

     

> About creating a CompilerDecl class, I agree with you. I used CompilerDeclContext since it was just a void* and it was convenient. Do you think that the FindVariable method should be in ClangASTContext since there is the need to actually interpret the CompilerDeclContext according to clang rules?


There will need to be a ClangASTContext::DeclContextXXXX call made, but this will need to be a TypeSystem virtual function that is overridden by ClangASTContext since it inherits from lldb_private::TypeSystem, but we should have an abstract call on CompilerDeclContext that initiates the search (CompilerDeclContext::FindVariable(...)), which in turn calls the "TypeSystem::DeclContextFindVariable(void *)". This then lets you get back to the clang specific ways of finding things.

We might want to go a bit more generic and make a function like:

  class CompilerDeclContext
  {
      CompilerDeclList FindDecls(const char *name);
  }

Since we actually want to start returning more than 1 thing for a search by name and there might be a variable named "a" and a function named "a" in that decl context? Or two overloads of a function "a" in the decl context. So the question is: do we try and search for CompilerDecl objects in a CompilerDeclContext so we can find all things whose name is "a", or do we stick with more targeted things like "CompilerDeclContext::FindVariable(...)". If we go the FindVariable route, we can probably skip making the new CompilerDecl and CompilerDeclList classes and just return lldb::VariableSP or use a lldb_private::VariableList class.

> Regarding ParseImportedNamespace, it also deals with "using NS::var" directives. Is there a method similar to FindNamespace that I can use?


We will need to add this to the CompilerDeclContext, TypeSystem and to the SymbolVendor/SymbolFile interfaces, all in abstract ways.

> In SymbolFileDWARF::GetDeclContextDIEContainingDIE, is there a reason why DW_TAG_subprogram, DW_TAG_lexical_block, etc are not considered DeclContexts?


They currently are expecting it to ignore these things. The question is if that is correct or not. We will need to look at who is asking. At the very least we can add a bool that says "include_functions_blocks" and then include them in the response if true if there are too many places that are using this incorrectly. There are only 3 places that call this function so it wouldn't be hard to fix them up to do the right thing.

The other thing Sean Callanan and Jim Ingham wanted to pass along is we want lookups to happen in via the clang::ExternalASTSource function callbacks like:

  namespace clang {
    class ExternalASTSource : public RefCountedBase<ExternalASTSource> {
      virtual bool
      FindExternalVisibleDeclsByName(const DeclContext *DC, DeclarationName Name);
    }
  }

I didn't look too closely at how your lookups were done, I mostly looked at the overall CompilerDeclContext and DWARF parsing aspect, but that is their preference. These lookups are always very targeted and say "inside the decl context 'DC', find "Name".


http://reviews.llvm.org/D12658