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

[Dawn Perchik via lldb-commits]

dawn marked 6 inline comments as done.
dawn added a comment.

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?
2. How is it anymore clang specific than DeclContextFindDeclByName?

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).


================
Comment at: source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp:1467-1468
@@ +1466,4 @@
+                            // Filter out class/instance methods.
+                            if (decl_ctx.IsClassMethod(nullptr, nullptr, nullptr))
+                                continue;
+                            sc_func_list.Append(sym_ctx);
----------------
clayborg wrote:
> Why are we pulling out class methods here?
To save time - they're thrown out by the loop that actually adds the functions which follows this, so no point in spending time on them. 

================
Comment at: source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp:1505-1509
@@ +1504,7 @@
+                            // scope and shadows the other.
+                            fdi.m_func_decl_lvl =
+                                ast->DeclContextCountDeclLevels(compiler_decl_context.GetOpaqueDeclContext(),
+                                                                func_decl_context.GetOpaqueDeclContext(),
+                                                                &fdi.m_function_name,
+                                                                &fdi.m_copied_function_type);
+                        }
----------------
clayborg wrote:
> Seems like you actually want this to be something like:
> 
> ```
> fdi.m_func_decl_lvl = func_decl_context.Depth(compiler_decl_context);
> ```
> 
> This would be a function on CompilerDeclContext that would calculate the depth of a decl context if the first argument (compiler_decl_context) is contained within the object (func_decl_context). This would be a useful API to add to CompilerDeclContext. It can return an integer and return "-1" if the compiler_decl_context ins't contained within func_decl_context, or zero or greater if it is.
> 
> Then you would need to add more functions the CompilerDeclContext to get "fdi.m_function_name" and "fdi.m_copied_function_type".
> 
As mentioned in prior discussions, this API is basically what you are suggesting.  The name and type are not returned - they are optional inputs into the function to allow for languages which support using declarations (because using declarations need to check type and/or name).  For example, in Delphi where using declarations are not supported, these would default to nullptr.


================
Comment at: source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp:1517
@@ +1516,3 @@
+                    uint32_t num_indices = sc_func_list.GetSize();
+                    std::vector<FuncDeclInfo> fdi_cache;
+                    fdi_cache.reserve(num_indices);
----------------
clayborg wrote:
> wouldn't this be better as std::multimap<uint32_t, FuncDeclInfo> where the uint32_t is the depth? Then you can change your foo loop at 1531 to just grab the first entry in the map and iterate as long as the depth is the same...
That won't work because we can have functions at more than one level that we'll want to keep in our set.  Using std::multimap<type, FuncDeclInfo> might help to clean things up a little however - will give it a try.


Repository:
  rL LLVM

http://reviews.llvm.org/D15312