[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#304652, @dawn wrote:

> Thanks Greg!  To address your main point:
>
> > So either make it generic, or clang specific.
>
>
> DeclContextCountDeclLevels's interface follows DeclContextFindDeclByName (authored by Paul Herman).  If DeclContextCountDeclLevels is to change, then DeclContextFindDeclByName should also change for consistency.


Indeed it should be. It should return a vector of CompilerDecl objects.

> My take on this is that the code in ClangExpressionDeclMap::FindExternalVisibleDecls which calls these scope-based lookup routines should be moved out of ClangExpressionDeclMap.cpp into CompilerExpressionDeclMap.cpp, so the interfaces to DeclContextFindDeclByName/DeclContextCountDeclLevels should remain generic while the implementations are compiler specific.  That's way more than I'd like to take on as part of this patch however.


I would still like to see your new ClangASTContext::DeclContextCountDeclLevels() broken up into one or more functions that are more useful on CompilerDeclContext. In my previous comment I mentioned a new CompilerDeclContext that could be used to get the depth:

  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.

Not sure how to break out the other thing with the name and type, but thing about what would make a good API on CompilerDeclContext and what questions you might ask of it. The DeclContextCountDeclLevels seems to be too specific.

> Paul, any feedback from you on this?


We should be using our CompilerDecl and CompilerDeclContext objects. No one should be using opaque pointers outside the API unless you are making the objects or changing the contained decl/decl context.


Repository:
  rL LLVM

http://reviews.llvm.org/D15312