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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
