[Lldb-commits] [PATCH] D127702: Support logpoints in lldb-vscode

[Greg Clayton via Phabricator via lldb-commits]

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