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