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

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

Just a few nits and this is good to go



================
Comment at: lldb/tools/lldb-vscode/BreakpointBase.cpp:56
+    formatted += text.substr(0, backslash_pos).str();
+    // Skip the characters before (and include) '\'.
+    text = text.drop_front(backslash_pos + 1);
----------------



================
Comment at: lldb/tools/lldb-vscode/BreakpointBase.cpp:29
+lldb::SBError BreakpointBase::AppendLogMessagePart(llvm::StringRef part,
+                                                          bool is_expr) {
+  if (is_expr) {
----------------
yinghuitan wrote:
> 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.
no, it is fixed now, so all good.


================
Comment at: lldb/tools/lldb-vscode/BreakpointBase.cpp:53
+    }
+    assert(backslash_pos >=0 && backslash_pos < text.size());
+
----------------
yinghuitan wrote:
> 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.
I would remove it because it is already part of the StringRef::find() contract and it isn't needed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136697