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

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

I would like to get to one array solution of LogMessagePart entries and avoid having two arrays of strings that need to stay in sync (rawTextMessages and evalExpressions). Simpler to understand and no need to document the interdependence of rawTextMessages and evalExpressions.



================
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(
----------------
yinghuitan wrote:
> 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);
> ```
I would like to avoid having two arrays that need to stay in sync here if possible. See comment on using just a single array of "LogMessagePart" entries. Simpler code without the need to document that rawTextMessages and evalExpressions need to be in some specific order.


================
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];
----------------
yinghuitan wrote:
> 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.
There is only one array and there is no need to document that the usage of two arrays and their codependence, such as you are doing with invariants above:
```
assert(bp->rawTextMessages.size() == bp->evalExpressions.size() + 1);
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127702/new/

https://reviews.llvm.org/D127702