yinghuitan added inline comments.
================
Comment at: lldb/tools/lldb-vscode/BreakpointBase.cpp:29
+lldb::SBError BreakpointBase::AppendLogMessagePart(llvm::StringRef part,
+ bool is_expr) {
+ if (is_expr) {
----------------
clayborg wrote:
> indentation is off here.
Hmm, it is chosen by clang-format. I tried again it is still the same result. I
will leave the decision to clang-format.
================
Comment at: lldb/tools/lldb-vscode/BreakpointBase.cpp:53
+ }
+ assert(backslash_pos >=0 && backslash_pos < text.size());
+
----------------
clayborg wrote:
> Remove assert, this should be already tested in StringRef::find(...)
I know but I like to use assertion as a contract documentation here so that it
is easier to reason later code. I can remove if you feel strong about it.
================
Comment at: lldb/tools/lldb-vscode/BreakpointBase.cpp:55
+
+ formatted += text.substr(0, backslash_pos + 1).str();
+ // Skip the characters before (and include) '\'.
----------------
clayborg wrote:
> Is this going to append the backslash character as well? Seems like a bug.
> Above suggested code did this correctly I think with:
>
> ```
> // Append anything before the backslash character.
> if (backslash_pos > 0)
> formatted += text.drop_front(backslash_pos).str();
> ```
> Which was always then followed by:
> ```
> text = text.drop_front(); // Drop the backslash
> ```
Thanks!
I wonder why the testcase did not catch it, turns out the testcase has a bug --
it uses escaped string for logmessage which did not trigger the code path at
all!!
Fixed the testcase caused this bug to show up.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D136697/new/
https://reviews.llvm.org/D136697
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
