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