yinghuitan added inline comments.
Herald added a subscriber: Michael137.
================
Comment at:
lldb/test/API/tools/lldb-vscode/breakpoint/TestVSCode_logpoints.py:60
+ [loop_line, after_loop_line],
+ [{'logMessage': logMessage}, {}]
+ )
----------------
clayborg wrote:
> Any reason we have an empty dictionary at the end here?
We are setting two source line breakpoints here, the first one has logMessage
while the second one does not have any condition. I am explicitly using {} for
second breakpoint to be clear.
================
Comment at:
lldb/test/API/tools/lldb-vscode/breakpoint/TestVSCode_logpoints.py:120
+ [loop_line, after_loop_line],
+ [{'logMessage': logMessage}, {}]
+ )
----------------
clayborg wrote:
> Any reason we have an empty dictionary at the end here?
{} means no logMessage for second breakpoint (after_loop_line).
================
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;
+
----------------
clayborg wrote:
> Might be easier to use a std::map<int, int> here instead of a vector of pairs?
I do not think so. We never need fast key lookup from the data structure. All
the operations are checking/popping/pushing the back of the vector. It is more
like a stack.
================
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(
----------------
clayborg wrote:
> 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)?
Correct. We assume there is always a prefix raw text before any expressions, so
rawTextMessages is always one element larger than expressions, I will enhance
the documentation (see my format comment above):
```
assert(rawTextMessages.size() == evalExpressions.size() + 1);
```
================
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];
----------------
clayborg wrote:
> 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();
> }
> }
Can you elaborate how is the suggested code simpler than what we have here? The
only difference seems to be that the suggested code added a if...else branch
while the code here does not branch? Seems not much difference.
================
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];
----------------
clayborg wrote:
> This will crash if "value.GetValue()" returns NULL.
Good catch, will handle 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
----------------
clayborg wrote:
> 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.
Sorry, I think I forgot to document this. The format of the logMessage is
parsed as:
```
prefixRawText/expression1/rawText1/expression2/rawText2..../expressionN/rawTextN
```
Basically rawTextMessages wraps expressions. This is enforced by invariant
`assert(rawTextMessages.size() == evalExpressions.size() + 1);` in the cpp
file.
Let me add this as documentation.
================
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);
----------------
clayborg wrote:
> Why was this changed? I seemed clearer before this change and I can't see
> anything that is different here?
This change is required because BreakpointBase::SetBreakpoint() will trigger
BreakpointBase::SetLogMessage() which stores this pointer for
`BreakpointBase::BreakpointHitCallback` callback access now. This means
SetBreakpoint() can't be called on Breakpoint object on stack (src_bp is on
stack). The new code solves the issue by storing stack object src_bp into
`g_vsc.source_breakpoints` (on heap) then call SetBreakpoint() on the heap
object.
================
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);
----------------
clayborg wrote:
> Why was this changed? Revert?
The same see my comment above.
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
