[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/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'])
----------------
clayborg wrote:
> 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
Per discussion offline with Greg, the SBValue::Persist() does not seem to freeze dry the struct so will revisit in future.


================
Comment at: lldb/tools/lldb-vscode/VSCode.cpp:33
 VSCode::VSCode()
-    : variables(), broadcaster("lldb-vscode"), num_regs(0), num_locals(0),
-      num_globals(0), log(),
+    : broadcaster("lldb-vscode"),
       exception_breakpoints(
----------------
wallace wrote:
> probably you shouldn't have removed log()
You should talk to Greg. He suggest removing above. For me, I really do not think it matters.


================
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++);
+}
----------------
wallace wrote:
> clayborg wrote:
> > use if statement. Easier to read
> +1
I am ok either way, but I think it is purely a preference. Forcing people to use one than the other is wasting time/energy. 


================
Comment at: lldb/tools/lldb-vscode/VSCode.h:83
 
+struct Variables {
+  // Bit mask to tell if a variableReference is inside
----------------
wallace wrote:
> Move it to a new file and add a global comment explaining the different kinds of variables that exist
I can do this in future refactoring. I am doing pragmatic refactoring here without delaying fixing the important bug.


================
Comment at: lldb/tools/lldb-vscode/VSCode.h:92-93
+
+  int64_t next_temporary_var_ref{VARREF_FIRST_VAR_IDX};
+  int64_t next_permanent_var_ref{VARREF_FIRST_VAR_IDX};
+  llvm::DenseMap<int64_t, lldb::SBValue> expandable_variables;
----------------
wallace wrote:
> if we are already distinguishing by the bitmask field, then I don't think we need to have these two counters. Just one should be enough.
I know I can save a variable but it is clear to keep separate counter for these two categories so that they have continuous value.


================
Comment at: lldb/tools/lldb-vscode/VSCode.h:94
+  int64_t next_permanent_var_ref{VARREF_FIRST_VAR_IDX};
+  llvm::DenseMap<int64_t, lldb::SBValue> expandable_variables;
+  llvm::DenseMap<int64_t, lldb::SBValue> expandable_permanent_variables;
----------------
wallace wrote:
> maybe call this expandable_temporary_variables to distinguish it from the next variable, otherwise it seems that the permanent ones are a subset of the other one
I can add a comment to make it clear. Greg is against long names.


================
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;
+
----------------
clayborg wrote:
> 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.
I understand that. But in modern development world, it should be the job of tools/IDE to format keep the 80 columns guideline. I do not like to scarifies the readability to short function/variable names just to meet 80 columns. If the function/variable is long formatter/IDE will wrap it not a big deal.


================
Comment at: lldb/tools/lldb-vscode/VSCode.h:113-114
+
+  /// Clear all scope variables and non-permanent expandable variables.
+  void Clear();
+};
----------------
wallace wrote:
> Rename this to ClearCurrentScopeVariables or ClearTemporaryVariables that to be more explicit
Talk to Greg who suggested short function name. Again, everyone has different opinion on this.


================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:110
 
+lldb::SBValueList *GetTopLevelScope(int64_t variablesReference) {
+  switch (variablesReference) {
----------------
wallace wrote:
> I don't it's necessary to pass a pointer, a SBValueList should be pretty cheap to copy/move around. It already has a bool operator () that you can use to check if the variable is valid or not, so your if statements should work well
We can, but the current approach works so I prefer to keep it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105166