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/breakpoint/TestVSCode_logpoints.py:60
+ [loop_line, after_loop_line],
+ [{'logMessage': logMessage}, {}]
+ )
----------------
Any reason we have an empty dictionary at the end here?
================
Comment at:
lldb/test/API/tools/lldb-vscode/breakpoint/TestVSCode_logpoints.py:120
+ [loop_line, after_loop_line],
+ [{'logMessage': logMessage}, {}]
+ )
----------------
Any reason we have an empty dictionary at the end here?
================
Comment at: lldb/tools/lldb-vscode/BreakpointBase.cpp:46
+ // ascending order without any overlap between them.
+ std::vector<std::pair<int, int>> matched_curly_braces_ranges;
+
----------------
Might be easier to use a std::map<int, int> here instead of a vector of pairs?
================
Comment at: lldb/tools/lldb-vscode/BreakpointBase.cpp:91
+ assert(curly_braces_range.first >= last_raw_text_start);
+ size_t raw_text_len = curly_braces_range.first - last_raw_text_start;
+ rawTextMessages.emplace_back(llvm::StringRef(
----------------
If the is a '{' at the start of the string, this code seems like it pushes an
empty string. this must be needed because we have two vectors (rawTextMessages
and evalExpressions)?
================
Comment at: lldb/tools/lldb-vscode/BreakpointBase.cpp:124
+
+ for (size_t i = 0; i < bp->evalExpressions.size(); ++i) {
+ const llvm::StringRef &expr = bp->evalExpressions[i];
----------------
If we use a single vector of LogMessagePart objects, this code becomes simpler:
```
for (const LogMessagePart &part: bp->logMessageParts) {
if (part.is_expr) {
lldb::SBValue value = frame.EvaluateExpression(part.text.str().c_str());
const char *expr_result = value.GetValue();
if (expr_result)
output += expr_result;
} else {
output += part.text.str();
}
}
================
Comment at: lldb/tools/lldb-vscode/BreakpointBase.cpp:128
+ lldb::SBValue value = frame.EvaluateExpression(expr.str().c_str());
+ output += value.GetValue();
+ output += bp->rawTextMessages[i + 1];
----------------
This will crash if "value.GetValue()" returns NULL.
================
Comment at: lldb/tools/lldb-vscode/BreakpointBase.h:31-32
std::string logMessage;
+ std::vector<llvm::StringRef> rawTextMessages;
+ std::vector<llvm::StringRef> evalExpressions;
// The LLDB breakpoint associated wit this source breakpoint
----------------
How do we know what order to emit things with here between "rawTextMessages"
and "evalExpressions" in the breakpoint hit callback?
================
Comment at: lldb/tools/lldb-vscode/BreakpointBase.h:31-32
std::string logMessage;
+ std::vector<llvm::StringRef> rawTextMessages;
+ std::vector<llvm::StringRef> evalExpressions;
// The LLDB breakpoint associated wit this source breakpoint
----------------
clayborg wrote:
> How do we know what order to emit things with here between "rawTextMessages"
> and "evalExpressions" in the breakpoint hit callback?
Seems like it would be easier to understand the code if we used:
```
struct LogMessagePart {
llvm::StringRef text;
bool is_expr;
};
std::vector<LogMessagePart> logMessageParts;
```
Right now you have two arrays and it isn't clear that each of these arrays must
be the same size, or which one would be emitted first.
================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:2084-2087
+ g_vsc.source_breakpoints[path][src_bp.line] = src_bp;
+ SourceBreakpoint &new_bp = g_vsc.source_breakpoints[path][src_bp.line];
+ new_bp.SetBreakpoint(path.data());
+ AppendBreakpoint(new_bp.bp, response_breakpoints, path, new_bp.line);
----------------
Why was this changed? I seemed clearer before this change and I can't see
anything that is different here?
================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:2312-2314
+ FunctionBreakpoint &new_bp = g_vsc.function_breakpoints[pair.first()];
+ new_bp.SetBreakpoint();
+ AppendBreakpoint(new_bp.bp, response_breakpoints);
----------------
Why was this changed? Revert?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D127702/new/
https://reviews.llvm.org/D127702
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
