[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/VSCode.cpp:531
 
+void Variables::Clear() {
+  locals.Clear();
----------------
clayborg wrote:
> 
I am not sure. I like to call it Clear() in cause we will need to call it from other scenario beyond process continue.


================
Comment at: lldb/tools/lldb-vscode/VSCode.cpp:535
+  registers.Clear();
+  expandable_variables.clear();
+}
----------------
clayborg wrote:
> Do we want to set "next_temporary_var_ref" to VARREF_FIRST_VAR_IDX here too? Ok if we don't.
Good question. I guess we can reset it if wanted but not matter much though.


================
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;
----------------
clayborg wrote:
> rename to "temporary_variables" and add a comment
I kind of disagree with this. I think the keyword "expandable" is very important for reading. It immediately tells reader that the map only contains expandable variables instead of any other variables. 



================
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:
> rename to "permanent_variables" and add comment
The same as my comment above.


================
Comment at: lldb/tools/lldb-vscode/VSCode.h:114
+  /// Clear all scope variables and non-permanent expandable variables.
+  void Clear();
+};
----------------
clayborg wrote:
> clear sounds like you are clearing everything in the class. I would name this willContinue() like the others
I am not sure. I like to call it Clear() in cause we will need to call it from other scenario beyond process continue. I do not think reader will be confused by permanent map is not cleared because the name permanent imply that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105166