[Lldb-commits] [PATCH] D136697: Add formatting support for VSCode logpoints message

[jeffrey tan via Phabricator via lldb-commits]

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