[Lldb-commits] [PATCH] D15312: Fix scope-based lookup when more than one function is found.

[Greg Clayton via lldb-commits]

clayborg added a comment.

In http://reviews.llvm.org/D15312#305392, @dawn wrote:

> > Greg: But going forward I would like to see more of "find a struct named 'X'
>
>
> in CompilerDeclContext 'Y'" queries, instead of ...
>
> I think lldb should have both, a "search for anything named foo in my scope context" (for when a user does "p foo"), and a "search for a thingy (functions/types/variables/etc.) named foo in my scope context" (for when a user does "expr --type function -- foo").


That is fine.

> 

> 

> > tberghammer: I am not sure if doing all of the parsing lazily is the good approach because of speed considerations.  ...  looking into the direction we are heading now we will need to parse more and more information to improve the expression evaluation

> 

> 

> Agreed.

> 

> > Greg:

> 

> > 

> 

> >   int32_t CompilerDeclContext::GetDepth (const CompilerDeclContext &decl_ctx);

> 

> > 

> 

> > This would get the depth of one CompilerDeclContext within another CompilerDeclContext, it would return -1 if "decl_ctx" doesn't fall within the object, and zero or above it is is contained.

> 

> 

> The problem with this is that it won't work for using declarations which require information about the thing we're looking up before it can determine the proper scope levels.

> 

> > BTW: I fixed DeclContextFindDeclByName to return a vector of CompilerDecl objects:

> 

> 

> Cool, but it needs to accept an optional type to deal with function overloading.  I can add that and see if I can come up with a new patch that uses the new DeclContextFindDeclByName. (In my copious spare time - heh).  But...


No rush for sure...

> One major performance benefit of my original implementation is that the function's name and type *only* need to be tested in the case of a using declaration, so in general it's much faster, because we only need to look for the function's parent's lookup scope.  So, are you sure you want to go down this path?


If you want to make this patch general then yes, but I wouldn't recommend it.

The functionality that you need is very specific to clang and your original implementation it just fine, I would say to just make a clang specific function that does exactly what you need and is only on ClangASTContext. We have discovered by our conversations above that the stuff you want to do doesn't map well onto CompilerDeclContext or CompilerDecl. Our needs are just too specific. So I vote to remove all code from TypeSystem.h and GoASTContext.h, change the version in ClangASTContext.h to be:

  uint32_t
  CountDeclLevels (clang::DeclContext *decl_ctx,
                               clang::DeclContext *child_decl_ctx,
                               ConstString *find_name = nullptr,
                               CompilerType *find_type = nullptr) override;

And just do what you need to do as efficiently as you need to. When you are in ClangExpressionDeclMap.cpp you know that your type system is a ClangASTContext already, so you should be able to just use your existing AST.

My whole aversion to the previous solution was you were putting very specify clang stuff into an API (TypeSystem) that is designed to be language agnostic.


Repository:
  rL LLVM

http://reviews.llvm.org/D15312