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

[Greg Clayton via lldb-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

So one of two things needs to happen here:

- ClangASTContext::DeclContextCountDeclLevels() becomes a function that is on ClangASTContext only and the opaque arguments get changed into "clang::DeclContext *" args, then remove all DeclContextCountDeclLevels functions from TypeSystem.h and GoASTContext.h.
- Change ClangASTContext::DeclContextCountDeclLevels() over to use CompilerDeclContext objects for both "void *opaque_decl_ctx" and "void *opaque_find_decl_ctx" and add any functions you need to do CompilerDeclContext that is needed to complete the functionality of this function without downcasting to clang objects.

So either make it generic, or clang specific.

I would vote for the first method since this functionality is very clang and C++ specific. It doesn't seem like a nice general purpose function any other language would use. If you do go the second route, you really need to abstract the heck out of this and add any needed methods to CompilerDeclContext.


================
Comment at: source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp:1443
@@ +1442,3 @@
+                    comp_sym_ctx = frame->GetSymbolContext(lldb::eSymbolContextFunction|lldb::eSymbolContextBlock);
+                CompilerDeclContext compiler_decl_context = comp_sym_ctx.block != nullptr ? comp_sym_ctx.block->GetDeclContext() : CompilerDeclContext();
+
----------------
"compiler_decl_context" should probably be named "frame_decl_ctx" for clarity

================
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);
----------------
Why are we pulling out class methods here?

================
Comment at: source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp:1485
@@ +1484,3 @@
+                    };
+                    auto initFuncDeclInfo = [this, compiler_decl_context, ast](const SymbolContext &sym_ctx)
+                    {
----------------
There is no need for this lamba, inline the code at the one and only place that it is used.

================
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);
+                        }
----------------
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".


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

================
Comment at: source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp:1524
@@ +1523,3 @@
+
+                        struct FuncDeclInfo fdi = initFuncDeclInfo(sym_ctx);
+                        fdi_cache.emplace_back(fdi);
----------------
inline the initFuncDeclInfo lambda here without needing a lambda


Repository:
  rL LLVM

http://reviews.llvm.org/D15312