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

[Raphael Isemann via Phabricator via lldb-commits]

teemperor requested changes to this revision.
teemperor added inline comments.
This revision now requires changes to proceed.


================
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):
----------------
I believe `gmodules` is unnecessary (or I'm missing something) :)


================
Comment at: packages/Python/lldbsuite/test/expression_command/namespace_local_var_same_name_cpp_and_c/main.cpp:7
+  void foo() {
+    int error=1; 
+
----------------
Can you clang-format this file (and all the other cpp files too just to be safe)?


================
Comment at: source/Expression/ExpressionSourceCode.cpp:172
+
+    // We can check for .block_descriptor w/o checking for langauge since this
+    // is not a valid identifier in either C or C++.
----------------
langauge -> language


================
Comment at: source/Expression/ExpressionSourceCode.cpp:256
-    if (add_locals) {
-      if (Language::LanguageIsCPlusPlus(frame->GetLanguage())) {
-        if (target->GetInjectLocalVariables(&exe_ctx)) {
----------------
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.


================
Comment at: source/Expression/ExpressionSourceCode.cpp:339
             "{                                                       \n"
+            "    %s;                                \n"
             "%s"
----------------
I think the end of this string should be aligned with the others.


================
Comment at: source/Expression/ExpressionSourceCode.cpp:353
             "{                                                      \n"
+            "    %s;                                \n"
             "%s"
----------------
Again, end of the string should be aligned.


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

https://reviews.llvm.org/D59960