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

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.


================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:2784
         if (curr_variable.MightHaveChildren())
-          newVariablesReference = i;
+          newVariablesReference = g_vsc.variables.GetNewVariableRefence(true);
         break;
----------------
clayborg wrote:
> We can't just make up a variable reference here, it needs to match the variable reference for "curr_variable" when it was first inserted. It used to be set to "i" before since that _was_ the actualy variable reference of the item we are changing the value of. It is fine for this to be 0 if we are setting say a local variable that is a "int x" since it has no variable reference, but we do need the variable reference to match the original.
> 
> So, we need to track the ID given to each local, global, or register variable and map it to a variable reference. 
> 
> I am not really sure why the result of this needs to return the new variable reference, or how it is used in the IDE.
The quicker way to fix this without having to track a map of lldb::SBValue -> var_ref would be to always insert it again into our temp lists:

```
newVariablesReference = g_vsc.variables.InsertExpandableVariable(variable, false);
```
We would end up with multiple entries for this variable, but that should be ok as they will get cleared out the next time we resume/stop with the Clear() method...


================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:2812
+            g_vsc.variables.IsPermanentVariableReference(variablesReference);
+        g_vsc.variables.InserExpandableVariable(variable, is_permanent);
       }
----------------
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()"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105166