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