[Lldb-commits] [PATCH] D85145: Use syntax highlighting also in gui mode

[Raphael Isemann via Phabricator via lldb-commits]

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

In D85145#2191658 <https://reviews.llvm.org/D85145#2191658>, @llunak wrote:

> In D85145#2191421 <https://reviews.llvm.org/D85145#2191421>, @teemperor wrote:
>
>> Btw, the highlighter supports any kind of delimiter string when 'highlighting' source. So you could add a parameter for a custom highlighter too and then pass a more convenient highlighter 'style' in to make the parsing simpler. See the only call MakeVimStyle (which generates the style that adds the color escapes) and the HighlighterStyle where you can set any kind of delimiter.
>
> I think I don't want to do that. The gui mode should preferably use the same highlighting as the non-gui one, so if I added a new style, the colors would still need to be mapped somewhen. Moreover the ^[<color>m style parser is actually pretty simple, much simpler than I was originally afraid it'd be, and possibly it could be later needed for something else too.

Yeah I just scrolled over the code and thought that could be simplified with dedicated format, but it seems the parsing logic is quite simple. The color parser could indeed be useful for getting colors to work on legacy Windows terminals, so that seems useful. I only have some small requests to the parsing implementation but otherwise this looks good to me. Thanks for working on this!

I wonder if there is a reasonable way to test this. From what I understand these attributes aren't in any output buffer that we could expect (e.g., with a pexpect test).



================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:461
+  // convert color escape sequences to curses color attributes.
+  void OutputColoredStringTruncated(int right_pad, const StringRef &string,
+                                    bool blue) {
----------------
StringRef is usually passed by-value. Also I think the `Truncated` suffix is what was used in other methods to indicate that it doesn't output a full CString, but here we anyway don't use C-Strings (yay).


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:462
+  void OutputColoredStringTruncated(int right_pad, const StringRef &string,
+                                    bool blue) {
+    int last_text = -1;  // Last position in string that's been written.
----------------
`blue` -> `use_blue_background` ?


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:471
+      ::wattron(m_window, COLOR_PAIR(16));
+    for (size_t i = 0; i < string.size(); ++i) {
+      if (string[i] != '\x1b') { // esc
----------------
StringRef has a bunch of parsing utilities (consume_front, etc.) that could help here (not tested if that code actually works, so maybe needs some fixes):

```
lang=c++
llvm::StringRef left_to_parse = string;
while (!left_to_parse.empty()) {
  if (!left_to_parse.consume_front("\x1b")) {
    ++text_length;
    continue;
  }
  [...]
  if (!left_to_parse.consume_front("["))
   return llvm::createStringError(llvm::inconvertibleErrorCode(),
                                   "Missing '[' in color escape sequence");
  unsigned value;
  if (left_to_parse.consumeInteger(10, value))
   return llvm::createStringError(llvm::inconvertibleErrorCode(),
                                  "No valid color code in color escape sequence");
  if (!left_to_parse.consume_front("m"))
   return llvm::createStringError(llvm::inconvertibleErrorCode(),
                                  "No 'm' in color escape sequence");
  [...]
}
```

I just returned an llvm::Error here as it seems more appropriate for parsing errors. You can handle it in the caller with something like that:
```
lang=c++
        handleAllErrors(
            std::move(result_from_call)
            [&](StringError &e) { llvm::errs() << "Error while highlighting source: " << e.getMessage() << "\n"; },
```


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:475
+        continue;
+      } else {
+        if (text_length > 0) {
----------------
else after continue


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:492
+        }
+        const int value = atoi(string.data() + esc_start);
+        if (value == 0) { // Reset.
----------------
There is also `llvm::to_integer` (and then you could also assert on an successful parse as it doesn't use magic return values for errors).


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:1002
     bool underlined_shortcut = false;
-    const attr_t hilgight_attr = A_REVERSE;
+    const attr_t highlight_attr = A_REVERSE;
     if (highlight)
----------------
You can just land typo fixes like this without review.


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:3632
+          StringRef line = lineStream.GetString();
+          if (!line.empty() && line.back() == '\n') // remove trailing \n
+            line = StringRef(line.data(), line.size() - 1);
----------------
```
lang=c++
if (line.endswith("\n"))
  line = line.drop_back();
```


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D85145