[Lldb-commits] [PATCH] D59960: Fix for ambiguous lookup in expressions between local variable and namespace

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik marked 2 inline comments as done.
shafik added inline comments.


================
Comment at: packages/Python/lldbsuite/test/expression_command/namespace_local_var_same_name_cpp_and_c/TestNamespaceLocalVarSameNameCppAndC.py:11
+    @skipUnlessDarwin
+    @add_test_categories(["gmodules"])
+    def test_namespace_local_var_same_name_cpp_and_c(self):
----------------
teemperor wrote:
> I believe `gmodules` is unnecessary (or I'm missing something) :)
This only reproduces in modules build. The inconsistency is a separate issue.


================
Comment at: source/Expression/ExpressionSourceCode.cpp:256
-    if (add_locals) {
-      if (Language::LanguageIsCPlusPlus(frame->GetLanguage())) {
-        if (target->GetInjectLocalVariables(&exe_ctx)) {
----------------
teemperor wrote:
> I believe this check was originally there because injecting local variables into the expression is quite costly (as we will have to preload all the associated AST nodes even if they are unused).
> 
> It seems this patch now completely removes this check, even though we don't always need to inject variables for this fix (e.g. for the C wrapping language, which is for functions in C and I believe also non-member functions in C++). I think the `AddLocalVariableDecls` should do the checking and do nothing unless we are in C++, Obj-C or Obj-C++. Otherwise this patch could degrade performance in these other cases as an unintended side effect.
This is a good point. I am going to be working on getting this fix in place which should address this issue https://reviews.llvm.org/D46551


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59960/new/

https://reviews.llvm.org/D59960