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



_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to