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

[Greg Clayton via Phabricator via lldb-commits]

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


================
Comment at: lldb/test/API/tools/lldb-vscode/variables/TestVSCode_variables.py:53
             if varRef != 0 and varref_dict is not None:
-                varref_dict[actual['evaluateName']] = varRef
+                evaluateName = expression if expression is not None else actual['evaluateName']
+                varref_dict[evaluateName] = varRef
----------------
Don't use 1 line if statements, hard to read and it is over 80 columns


================
Comment at: lldb/test/API/tools/lldb-vscode/variables/TestVSCode_variables.py:332
+
+        # 2. Evluate expandable expression twice: once permanent (from repl) the other temporary (from other UI).
+        expandable_expression = {
----------------
a/Evluate/Evaluate/


================
Comment at: lldb/test/API/tools/lldb-vscode/variables/TestVSCode_variables.py:333-339
+        expandable_expression = {
+            'pt': {
+                'equals': {'type': 'PointType'},
+                'startswith': {'result': 'PointType @ 0x'},
+                'hasVariablesReference': True
+            },
+        }
----------------
Why is this here? this has the same name as the next variable.


================
Comment at: lldb/test/API/tools/lldb-vscode/variables/TestVSCode_variables.py:386-389
+        var_ref = permanent_expr_varref_dict[expandable_expression['name']]
+        response = self.vscode.request_variables(var_ref)
+        self.verify_variables(expandable_expression['children'],
+                                  response['body']['variables'])
----------------
When we are stopped here, has the actual "pt" value been updated? I would like the test to have the current value of "pt" differ from the freeze dried version from the 'repl'. We need to make sure the value hasn't changed, but the real 'pt' has


================
Comment at: lldb/tools/lldb-vscode/VSCode.cpp:540-541
+int64_t Variables::GetNewVariableRefence(bool is_permanent) {
+  return is_permanent ? (PermanentVariableBitMask | next_permanent_var_ref++)
+                      : (next_temporary_var_ref++);
+}
----------------
use if statement. Easier to read


================
Comment at: lldb/tools/lldb-vscode/VSCode.cpp:563
+                                               bool is_permanent) {
+  auto var_ref = GetNewVariableRefence(is_permanent);
+  if (is_permanent)
----------------
don't use auto, nice to see what the type is: int64_t


================
Comment at: lldb/tools/lldb-vscode/VSCode.h:95
+  llvm::DenseMap<int64_t, lldb::SBValue> expandable_variables;
+  llvm::DenseMap<int64_t, lldb::SBValue> expandable_permanent_variables;
+
----------------
my main point here is that really long variable names make it hard to write code that fits into 80 columns. So check where this is used in the code and make sure you aren't running into 80 cols a lot.  If you are, consider shortening it.


================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:1232
       if (value.MightHaveChildren()) {
-        auto variablesReference = VARIDX_TO_VARREF(g_vsc.variables.GetSize());
-        g_vsc.variables.Append(value);
-        body.try_emplace("variablesReference", variablesReference);
+        auto variableReference = g_vsc.variables.InsertNewExpandableVariable(
+            value, /*is_permanent=*/context == "repl");
----------------
Example of long function names making 80 columns harder here. We could rename to just "Insert(..)"


================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:2946
+      if (variable.MightHaveChildren()) {
+        var_ref = g_vsc.variables.InsertNewExpandableVariable(
+            variable, /*is_permanent=*/false);
----------------
long function name, rename to just "Insert"?


================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:2965
         if (child.MightHaveChildren()) {
-          const int64_t var_idx = g_vsc.variables.GetSize();
-          auto childVariablesReferences = VARIDX_TO_VARREF(var_idx);
-          variables.emplace_back(
-              CreateVariable(child, childVariablesReferences, var_idx, hex));
-          g_vsc.variables.Append(child);
+          auto is_permanent =
+              g_vsc.variables.IsPermanentVariableReference(variablesReference);
----------------
don't use auto for bool


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105166