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

[Luboš Luňák via Phabricator via lldb-commits]

llunak added a comment.

In D85145#2192716 <https://reviews.llvm.org/D85145#2192716>, @teemperor wrote:

> 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).

I'm not sure. There's TERM hardcoded to vt100 in lldbpexpect, and vt100 is not color-capable. Grepping shows other references to vt100. TestGuiBasic.py matches "return 1", which should be different with syntax highlight, and it still works even with forcing that TERM to e.g. ansi or xterm-color. So at least not easily.



================
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) {
----------------
teemperor wrote:
> 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).
To my understanding that `Truncated` refers to making sure the text fits the curses window, including the padding.




================
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
----------------
teemperor wrote:
> 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"; },
> ```
> I just returned an llvm::Error here as it seems more appropriate for parsing errors.

Why? I went simply with assert() because it's meant to parse output from another part of LLDB, so it made sense to just make the code fall flat on its face in case of a problem. Guessing from your comments the output from the highlighter is not as hardcoded as I assumed, so it makes sense to handle it gracefully, but still that's a problem of the function and the caller neither cares nor can do much about it.



Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D85145