[Lldb-commits] [PATCH] D105166: Fix expression evaluation result expansion in lldb-vscode

[jeffrey tan via Phabricator via lldb-commits]

yinghuitan added inline comments.


================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:2812
+            g_vsc.variables.IsPermanentVariableReference(variablesReference);
+        g_vsc.variables.InserExpandableVariable(variable, is_permanent);
       }
----------------
clayborg wrote:
> clayborg wrote:
> > Rename to "Insert(...)" and you must use the variable reference that was returned.
> > 
> > That being said, the old code was incorrect as it was appending the same value again in the the "g_vsc.variables" list, but it didn't need to. It should have just been returning the old index of the for "variable" in the g_vsc.variables" list. So we will need a way to get the original variable reference for any "SBValue" back from the "g_vsc.variables" class. One idea is to rely on the following SBValue method:
> > ```
> > lldb::user_id_t SBValue::GetID();
> > ```
> > And when we call "g_vsc.variables.InserExpandableVariable(variable, is_permanent)" we have that method keep a map of "lldb::user_id_t -> var_ref". 
> Thinking about this some more, we should probably just go with the code fix suggestion I made above. This might insert the same "variable" into the lists again, but this only happens when we set a variable value, so this won't be too often. Another point is people don't often edit top level structures, they edit variables that have values. So if you have a "Point pt" variable, you won't usually try to edit top level "pt" value, you would edit the "pt.x" or "pt.y" values. If you do try to edit the top level structure value, it will probably return an error. So it is doubtful that this will even cause many problems since most items users will edit won't return "true" for a call to "variable.MightHaveChildren()"
This is a good point. However, reading this code more, I wonder if the current behavior is wrong -- in current behavior, after variable is located in parent scope/object, it is checked to be expandable or not and is added into expandable_variables list *before* new value assignment. I think the the correct behavior should be only checking and add to expandable_variables list *after* new value assignment, right?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105166