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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
