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

> Hi Greg, I'm working on a new revision to change the patch as you suggest (thanks for your review - you had some great suggestions!).
>
> Sorry, if I'm missing something obviously here, but there's still some things I don't understand:
>
> 1. What exactly was clang specific about the DeclContextCountDeclLevels API?


CountDeclLevels doesn't really tell me what the function does. For abstract APIs I would expect an API like:

  int CompilerDeclContext::GetDeclDepth (const CompilerDeclContext &child_decl_ctx);

That makes sense. It seems like you combined "find this decl instance within a decl context" with "find a decl by name in the CompilerDeclContext and optionally get the type". This is what I didn't like about API you added. It was too specialized and didn't make for good API on the CompilerDeclContext class. The class name "CountDeclLevels" didn't really say what this function would do for me. What levels? Is level 10 better than level 1? Why would I supply a name to this function if I am trying to find a specific decl (from opaque_find_decl_ctx)? Why does this function not work if I supply a type to fill in (CompilerType *find_type = <valid_ptr>) but not a name (ConstString *find_name == nullptr)? I am still unclear as to exactly what this function does even after reading it many times.

> 2. How is it anymore clang specific than DeclContextFindDeclByName?


This is generic. In any CompilerDeclContext, you can find one or more decls by name. That makes sense to me:

  CompilerDeclContext decl_ctx = ...;
  std::vector<CompilerDecl> decls = decl_ctx.FindDeclByName("hello");

That makes sense in any language.

> Anyway, I didn't want to drag this out any longer so am making the code clang specific.  We can always generalize it in the future (I want it for Delphi).


You sure can, you will need to make sure the APIs make sense though. I am still unclear as to what the name and type are doing in DeclContextCountDeclLevels. I don't see how we would ever have a decl (in opaque_find_decl_ctx) that isn't unique where the same decl could have different names and different types?


Repository:
  rL LLVM

http://reviews.llvm.org/D15312