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

Reply via email to