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