[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/test/API/tools/lldb-vscode/breakpoint/TestVSCode_logpoints.py:52
         logMessage_prefix = "This is log message for { -- "
-        # Trailing newline is needed for splitlines()
-        logMessage = logMessage_prefix + "{i + 3}\n"
+        logMessage = logMessage_prefix + "{i + 3}"
         [loop_breakpoint_id, post_loop_breakpoint_id] = self.set_source_breakpoints(
----------------
clayborg wrote:
> Are we now auto appending a newline if there isn't one? If so we need to test this functionality. We should append on if the string doesn't end with "\n" and not if it does?
Yes, we are always appending a newline now. I decided not to detect if string ends with "\n" or not but always append so that the behavior can be consistent. 

By removing the trailing the trailing newline in testcase here we are already testing the newline behavior right? Otherwise the testcase would fail.


================
Comment at: lldb/tools/lldb-vscode/BreakpointBase.cpp:47-52
+  while (!text.empty()) {
+    if (text[0] != '\\') {
+      formatted.push_back(text[0]);
+      text = text.drop_front();
+      continue;
+    }
----------------
clayborg wrote:
> Please use StringRef::find(char). Something like:
> 
> Please use StringRef::find_first_of(...) instead of iterating per character. Something like:
> 
> ```
> formatted.clear();
> while (!text.empty()) {
>   size_t backslash_pos = text.find_first_of('\\');
>   if (backslash_pos = StringRef::npos) {
>     // No more backslash chars append the rest of the string
>     formatted += text.str();
>     return error;
>   }
>   // Append anything before the backslash character.
>   if (backslash_pos > 0)
>     formatted += text.drop_front(backslash_pos).str();
> ```
Ah, definitely.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136697