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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
