[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/tools/lldb-vscode/VSCode.cpp:531
 
+void Variables::Clear() {
+  locals.Clear();
----------------



================
Comment at: lldb/tools/lldb-vscode/VSCode.cpp:535
+  registers.Clear();
+  expandable_variables.clear();
+}
----------------
Do we want to set "next_temporary_var_ref" to VARREF_FIRST_VAR_IDX here too? Ok if we don't.


================
Comment at: lldb/tools/lldb-vscode/VSCode.cpp:538
+
+int64_t Variables::GetNewVariableRefence(bool is_permanent) {
+  if (is_permanent)
----------------
rename


================
Comment at: lldb/tools/lldb-vscode/VSCode.cpp:549
+
+lldb::SBValue Variables::GetVariableFromVariableReference(int64_t var_ref) {
+  lldb::SBValue variable;
----------------
rename to "GetVariable" to help with 80 column limit and make const;


================
Comment at: lldb/tools/lldb-vscode/VSCode.cpp:550-555
+  lldb::SBValue variable;
+  if (IsPermanentVariableReference(var_ref))
+    variable = expandable_permanent_variables.find(var_ref)->second;
+  else
+    variable = expandable_variables.find(var_ref)->second;
+  return variable;
----------------
We need to guard against the variable reference not existing in the map to make sure we never crash:


================
Comment at: lldb/tools/lldb-vscode/VSCode.cpp:558
+
+int64_t Variables::InsertNewExpandableVariable(lldb::SBValue expandableVariable,
+                                               bool is_permanent) {
----------------
var name for LLDB and rename to shorter name.


================
Comment at: lldb/tools/lldb-vscode/VSCode.cpp:560
+                                               bool is_permanent) {
+  auto newVariablesReferences = GetNewVariableRefence(is_permanent);
+  if (is_permanent)
----------------
rename newVariablesReferences to "var_ref"


================
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;
----------------
rename to "temporary_variables" and add a comment


================
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;
+
----------------
rename to "permanent_variables" and add comment


================
Comment at: lldb/tools/lldb-vscode/VSCode.h:99
+  /// expandable_permanent_variables.
+  bool IsPermanentVariableReference(int64_t variableReference);
+
----------------
lldb variable name, and make static since it doesn't require any ivar access


================
Comment at: lldb/tools/lldb-vscode/VSCode.h:110
+  /// Insert a new \p expandableVariable.
+  int64_t InsertNewExpandableVariable(lldb::SBValue expandableVariable,
+                                      bool is_permanent);
----------------
Fix LLDB variable name


================
Comment at: lldb/tools/lldb-vscode/VSCode.h:114
+  /// Clear all scope variables and non-permanent expandable variables.
+  void Clear();
+};
----------------
clear sounds like you are clearing everything in the class. I would name this willContinue() like the others


================
Comment at: lldb/tools/lldb-vscode/VSCode.h:238
+  /// Debuggee will continue from stopped state.
+  void willContinue() { variables.Clear(); }
+
----------------
Correct camel case for LLDB function names


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105166