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

[Sterling Augustine via Phabricator via lldb-commits]

saugustine added a comment.

This change has a subtle isse with wattr_get and friends: saved_opts isn't 
actually used, and the documentation for them says to always pass a nullptr. 
"The parameter opts is reserved for future use, applications must supply a null 
pointer."

I fixed it in 9dbdaea9a0e6f58417b5bd8980e7ea6723fd1783 
.


Repository:
  rG LLVM Github Monorepo

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

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

[Martin Storsjö via Phabricator via lldb-commits]

mstorsjo added a comment.

In D85145#2199045 , @teemperor wrote:

> Same on Arch Linux. Should be fixed in 
> 45f9fc890705a8272e5253602f5506fdef4586e2 (I just removed the scope operator).

Indeed it is, thanks!


Repository:
  rG LLVM Github Monorepo

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

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

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

Same on Arch Linux. Should be fixed in 45f9fc890705a8272e5253602f5506fdef4586e2 
(I just removed the scope operator).


Repository:
  rG LLVM Github Monorepo

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

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

[Martin Storsjö via Phabricator via lldb-commits]

mstorsjo added a comment.
Herald added a subscriber: JDevlieghere.

This broke compilation on Ubuntu 18.04:

  In file included from ../tools/lldb/source/Core/IOHandlerCursesGUI.cpp:17:0:
  ../tools/lldb/source/Core/IOHandlerCursesGUI.cpp: In member function ‘void 
curses::Window::OutputColoredStringTruncated(int, llvm::StringRef, bool)’:
  ../tools/lldb/source/Core/IOHandlerCursesGUI.cpp:463:7: error: expected 
id-expression before ‘(’ token
   ::wattr_get(m_window, &saved_attr, &saved_pair, &saved_opts);
 ^
  ../tools/lldb/source/Core/IOHandlerCursesGUI.cpp:499:11: error: expected 
id-expression before ‘(’ token
   ::wattr_set(m_window, saved_attr, saved_pair, &saved_opts);
 ^
  ../tools/lldb/source/Core/IOHandlerCursesGUI.cpp:508:7: error: expected 
id-expression before ‘(’ token
   ::wattr_set(m_window, saved_attr, saved_pair, &saved_opts);
 ^


Repository:
  rG LLVM Github Monorepo

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

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

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

This revision was automatically updated to reflect the committed changes.
Closed by commit rG14406ca01fd3: [lldb][gui] use syntax highlighting also in 
gui mode (authored by llunak).
Herald added a project: LLDB.

Changed prior to commit:
  https://reviews.llvm.org/D85145?vs=282865&id=283498#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85145

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp

Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -396,11 +396,11 @@
 ::wbkgd(m_window, COLOR_PAIR(color_pair_idx));
   }
 
-  void PutCStringTruncated(int right_pad, const char *s) {
+  void PutCStringTruncated(int right_pad, const char *s, int len = -1) {
 int bytes_left = GetWidth() - GetCursorX();
 if (bytes_left > right_pad) {
   bytes_left -= right_pad;
-  ::waddnstr(m_window, s, bytes_left);
+  ::waddnstr(m_window, s, len < 0 ? bytes_left : std::min(bytes_left, len));
 }
   }
 
@@ -452,6 +452,62 @@
 return std::min(length, std::max(0, GetWidth() - GetCursorX() - 1));
   }
 
+  // Curses doesn't allow direct output of color escape sequences, but that's
+  // how we get source lines from the Highligher class. Read the line and
+  // convert color escape sequences to curses color attributes.
+  void OutputColoredStringTruncated(int right_pad, StringRef string,
+bool use_blue_background) {
+attr_t saved_attr;
+short saved_pair;
+int saved_opts;
+::wattr_get(m_window, &saved_attr, &saved_pair, &saved_opts);
+if (use_blue_background)
+  ::wattron(m_window, COLOR_PAIR(16));
+while (!string.empty()) {
+  size_t esc_pos = string.find('\x1b');
+  if (esc_pos == StringRef::npos) {
+PutCStringTruncated(right_pad, string.data(), string.size());
+break;
+  }
+  if (esc_pos > 0) {
+PutCStringTruncated(right_pad, string.data(), esc_pos);
+string = string.drop_front(esc_pos);
+  }
+  bool consumed = string.consume_front("\x1b");
+  assert(consumed);
+  UNUSED_IF_ASSERT_DISABLED(consumed);
+  // This is written to match our Highlighter classes, which seem to
+  // generate only foreground color escape sequences. If necessary, this
+  // will need to be extended.
+  if (!string.consume_front("[")) {
+llvm::errs() << "Missing '[' in color escape sequence.\n";
+continue;
+  }
+  // Only 8 basic foreground colors and reset, our Highlighter doesn't use
+  // anything else.
+  int value;
+  if (!!string.consumeInteger(10, value) || // Returns false on success.
+  !(value == 0 || (value >= 30 && value <= 37))) {
+llvm::errs() << "No valid color code in color escape sequence.\n";
+continue;
+  }
+  if (!string.consume_front("m")) {
+llvm::errs() << "Missing 'm' in color escape sequence.\n";
+continue;
+  }
+  if (value == 0) { // Reset.
+::wattr_set(m_window, saved_attr, saved_pair, &saved_opts);
+if (use_blue_background)
+  ::wattron(m_window, COLOR_PAIR(16));
+  } else {
+// Mapped directly to first 16 color pairs (black/blue background).
+::wattron(m_window,
+  COLOR_PAIR(value - 30 + 1 + (use_blue_background ? 8 : 0)));
+  }
+}
+::wattr_set(m_window, saved_attr, saved_pair, &saved_opts);
+  }
+
   void Touch() {
 ::touchwin(m_window);
 if (m_parent)
@@ -540,7 +596,7 @@
   void DrawTitleBox(const char *title, const char *bottom_message = nullptr) {
 attr_t attr = 0;
 if (IsActive())
-  attr = A_BOLD | COLOR_PAIR(2);
+  attr = A_BOLD | COLOR_PAIR(18);
 else
   attr = 0;
 if (attr)
@@ -970,14 +1026,14 @@
 
 if (m_key_name.empty()) {
   if (!underlined_shortcut && llvm::isPrint(m_key_value)) {
-window.AttributeOn(COLOR_PAIR(3));
+window.AttributeOn(COLOR_PAIR(19));
 window.Printf(" (%c)", m_key_value);
-window.AttributeOff(COLOR_PAIR(3));
+window.AttributeOff(COLOR_PAIR(19));
   }
 } else {
-  window.AttributeOn(COLOR_PAIR(3));
+  window.AttributeOn(COLOR_PAIR(19));
   window.Printf(" (%s)", m_key_name.c_str());
-  window.AttributeOff(COLOR_PAIR(3));
+  window.AttributeOff(COLOR_PAIR(19));
 }
   }
 }
@@ -989,7 +1045,7 @@
   Menu::Type menu_type = GetType();
   switch (menu_type) {
   case Menu::Type::Bar: {
-window.SetBackground(2);
+window.SetBackground(18);
 window.MoveCursor(0, 0);
 for (size_t i = 0; i < num_submenus; ++i) {
   Menu *menu = submenus[i].get();
@@ -1009,7 +1065,7 @@
 int cursor_x = 0;
 int cursor_y = 0;
 window.Erase();
-window.SetBackground(2);
+window.SetBackground(18);
 window.Box();

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

[Raphael Isemann via Phabricator via lldb-commits]

teemperor accepted this revision.
teemperor added a comment.
This revision is now accepted and ready to land.

Sorry was OOO.

The source code is user input, so you can have anything in it. LLDB will 
happily read and return any file contents as long as it matches the source 
path. Like, create a test.cpp, compile it, then just copy over some binary file 
to the source path. Not that we should do some complicated error handling in 
this case, but as long as we don't assert and end the whole debug session it's 
IMHO fine. This LGTM to me now, thanks for working on this!

In D85145#2192991 , @llunak wrote:

> I find some of the StringRef APIs flawed though: consume_front() returns true 
> on success, but consumeInteger() returns false; consume_front() modifies the 
> object, but drop_front() doesn't.

Yeah I had the same realization when I typed the example.


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

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

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

LGTM. Raphael? You have any more comments?


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

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

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

llunak added inline comments.



Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:486
+  if (!string.consume_front("[")) {
+llvm::errs() << "Missing '[' in color escape sequence.\n";
+continue;

clayborg wrote:
> So what will happen if we actually get these errors? Will it just print right 
> in the curses view? If so, that doesn't seem optimal.
The idea is that it should ideally never happen, it's technically an internal 
error of not handling what Highlighter produces. In the discussion above I 
mentioned that I originally used assert(). So I don't see a real problem, if 
this actually happens then it shouldn't matter how it shows, and lldb already 
shows other such messages (which is BTW why Ctrl+L to redraw the screen was one 
of my first patches).





Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:4255-4275
+init_pair(1, COLOR_BLACK, COLOR_BLACK);
+init_pair(2, COLOR_RED, COLOR_BLACK);
+init_pair(3, COLOR_GREEN, COLOR_BLACK);
+init_pair(4, COLOR_YELLOW, COLOR_BLACK);
+init_pair(5, COLOR_BLUE, COLOR_BLACK);
+init_pair(6, COLOR_MAGENTA, COLOR_BLACK);
+init_pair(7, COLOR_CYAN, COLOR_BLACK);

clayborg wrote:
> Maybe we should make #define for each init_pair to make our code more 
> readable?
> ```
> #define GUI_BLACK_BLACK 1
> #define GUI_RED_BLACK 2
> ...
> init_pair(GUI_BLACK_BLACK, COLOR_BLACK, COLOR_BLACK);
> init_pair(GUI_RED_BLACK, COLOR_BLACK, COLOR_BLACK);
> ...
> ```
> 
> I know it was using magic numbers before.
D85286



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

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

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.



Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:486
+  if (!string.consume_front("[")) {
+llvm::errs() << "Missing '[' in color escape sequence.\n";
+continue;

So what will happen if we actually get these errors? Will it just print right 
in the curses view? If so, that doesn't seem optimal.



Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:4255-4275
+init_pair(1, COLOR_BLACK, COLOR_BLACK);
+init_pair(2, COLOR_RED, COLOR_BLACK);
+init_pair(3, COLOR_GREEN, COLOR_BLACK);
+init_pair(4, COLOR_YELLOW, COLOR_BLACK);
+init_pair(5, COLOR_BLUE, COLOR_BLACK);
+init_pair(6, COLOR_MAGENTA, COLOR_BLACK);
+init_pair(7, COLOR_CYAN, COLOR_BLACK);

Maybe we should make #define for each init_pair to make our code more readable?
```
#define GUI_BLACK_BLACK 1
#define GUI_RED_BLACK 2
...
init_pair(GUI_BLACK_BLACK, COLOR_BLACK, COLOR_BLACK);
init_pair(GUI_RED_BLACK, COLOR_BLACK, COLOR_BLACK);
...
```

I know it was using magic numbers before.


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

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

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

llunak updated this revision to Diff 282865.
llunak added a comment.

Updated according to comments.

I find some of the StringRef APIs flawed though: consume_front() returns true 
on success, but consumeInteger() returns false; consume_front() modifies the 
object, but drop_front() doesn't.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D85145

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp

Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -391,11 +391,11 @@
 ::wbkgd(m_window, COLOR_PAIR(color_pair_idx));
   }
 
-  void PutCStringTruncated(int right_pad, const char *s) {
+  void PutCStringTruncated(int right_pad, const char *s, int len = -1) {
 int bytes_left = GetWidth() - GetCursorX();
 if (bytes_left > right_pad) {
   bytes_left -= right_pad;
-  ::waddnstr(m_window, s, bytes_left);
+  ::waddnstr(m_window, s, len < 0 ? bytes_left : std::min(bytes_left, len));
 }
   }
 
@@ -455,6 +455,62 @@
 return std::min(length, std::max(0, GetWidth() - GetCursorX() - 1));
   }
 
+  // Curses doesn't allow direct output of color escape sequences, but that's
+  // how we get source lines from the Highligher class. Read the line and
+  // convert color escape sequences to curses color attributes.
+  void OutputColoredStringTruncated(int right_pad, StringRef string,
+bool use_blue_background) {
+attr_t saved_attr;
+short saved_pair;
+int saved_opts;
+::wattr_get(m_window, &saved_attr, &saved_pair, &saved_opts);
+if (use_blue_background)
+  ::wattron(m_window, COLOR_PAIR(16));
+while (!string.empty()) {
+  size_t esc_pos = string.find('\x1b');
+  if (esc_pos == StringRef::npos) {
+PutCStringTruncated(right_pad, string.data(), string.size());
+break;
+  }
+  if (esc_pos > 0) {
+PutCStringTruncated(right_pad, string.data(), esc_pos);
+string = string.drop_front(esc_pos);
+  }
+  bool consumed = string.consume_front("\x1b");
+  assert(consumed);
+  UNUSED_IF_ASSERT_DISABLED(consumed);
+  // This is written to match our Highlighter classes, which seem to
+  // generate only foreground color escape sequences. If necessary, this
+  // will need to be extended.
+  if (!string.consume_front("[")) {
+llvm::errs() << "Missing '[' in color escape sequence.\n";
+continue;
+  }
+  // Only 8 basic foreground colors and reset, our Highlighter doesn't use
+  // anything else.
+  int value;
+  if (!!string.consumeInteger(10, value) || // Returns false on success.
+  !(value == 0 || (value >= 30 && value <= 37))) {
+llvm::errs() << "No valid color code in color escape sequence.\n";
+continue;
+  }
+  if (!string.consume_front("m")) {
+llvm::errs() << "Missing 'm' in color escape sequence.\n";
+continue;
+  }
+  if (value == 0) { // Reset.
+::wattr_set(m_window, saved_attr, saved_pair, &saved_opts);
+if (use_blue_background)
+  ::wattron(m_window, COLOR_PAIR(16));
+  } else {
+// Mapped directly to first 16 color pairs (black/blue background).
+::wattron(m_window,
+  COLOR_PAIR(value - 30 + 1 + (use_blue_background ? 8 : 0)));
+  }
+}
+::wattr_set(m_window, saved_attr, saved_pair, &saved_opts);
+  }
+
   void Touch() {
 ::touchwin(m_window);
 if (m_parent)
@@ -543,7 +599,7 @@
   void DrawTitleBox(const char *title, const char *bottom_message = nullptr) {
 attr_t attr = 0;
 if (IsActive())
-  attr = A_BOLD | COLOR_PAIR(2);
+  attr = A_BOLD | COLOR_PAIR(18);
 else
   attr = 0;
 if (attr)
@@ -977,14 +1033,14 @@
 
 if (m_key_name.empty()) {
   if (!underlined_shortcut && llvm::isPrint(m_key_value)) {
-window.AttributeOn(COLOR_PAIR(3));
+window.AttributeOn(COLOR_PAIR(19));
 window.Printf(" (%c)", m_key_value);
-window.AttributeOff(COLOR_PAIR(3));
+window.AttributeOff(COLOR_PAIR(19));
   }
 } else {
-  window.AttributeOn(COLOR_PAIR(3));
+  window.AttributeOn(COLOR_PAIR(19));
   window.Printf(" (%s)", m_key_name.c_str());
-  window.AttributeOff(COLOR_PAIR(3));
+  window.AttributeOff(COLOR_PAIR(19));
 }
   }
 }
@@ -996,7 +1052,7 @@
   Menu::Type menu_type = GetType();
   switch (menu_type) {
   case Menu::Type::Bar: {
-window.SetBackground(2);
+window.SetBackground(18);
 window.MoveCursor(0, 0);
 for (size_t i = 0; i < num_submenus; ++i) {
   Menu *menu = submenus[i].get();
@@ -1016,7 +1072,7 @@
 int cursor_x = 0;
 int cursor_y = 0;
 window.Erase();
-window.SetBackground(2);
+window.SetBackground(18);
 window.Box();
 for (size_

[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 , @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

[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 , @llunak wrote:

> In 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 ^[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(

[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#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 ^[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.


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

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

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a reviewer: teemperor.
teemperor added a comment.

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.


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

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

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

llunak created this revision.
llunak added a reviewer: clayborg.
llunak requested review of this revision.

Use the same functionality as the non-gui mode, the colors just need 
translating to curses colors.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D85145

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp

Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -391,11 +391,11 @@
 ::wbkgd(m_window, COLOR_PAIR(color_pair_idx));
   }
 
-  void PutCStringTruncated(int right_pad, const char *s) {
+  void PutCStringTruncated(int right_pad, const char *s, int len = -1) {
 int bytes_left = GetWidth() - GetCursorX();
 if (bytes_left > right_pad) {
   bytes_left -= right_pad;
-  ::waddnstr(m_window, s, bytes_left);
+  ::waddnstr(m_window, s, len < 0 ? bytes_left : std::min(bytes_left, len));
 }
   }
 
@@ -455,6 +455,61 @@
 return std::min(length, std::max(0, GetWidth() - GetCursorX() - 1));
   }
 
+  // Curses doesn't allow direct output of color escape sequences, but that's
+  // how we get source lines from the Highligher class. Read the line and
+  // convert color escape sequences to curses color attributes.
+  void OutputColoredStringTruncated(int right_pad, const StringRef &string,
+bool blue) {
+int last_text = -1;  // Last position in string that's been written.
+int text_length = 0; // Size of normal characters to be written.
+attr_t saved_attr;
+short saved_pair;
+int saved_opts;
+::wattr_get(m_window, &saved_attr, &saved_pair, &saved_opts);
+if (blue)
+  ::wattron(m_window, COLOR_PAIR(16));
+for (size_t i = 0; i < string.size(); ++i) {
+  if (string[i] != '\x1b') { // esc
+++text_length;
+continue;
+  } else {
+if (text_length > 0) {
+  PutCStringTruncated(right_pad, string.data() + last_text + 1,
+  text_length);
+  text_length = 0;
+}
+++i;
+// Our Highlighter doesn't use any other escape sequences.
+assert(string[i] == '[');
+++i;
+const int esc_start = i;
+while (string[i] != 'm') {
+  // No semicolons, our Highlighter doesn't use them.
+  assert(string[i] >= '0' && string[i] <= '9');
+  ++i;
+  assert(i < string.size());
+}
+const int value = atoi(string.data() + esc_start);
+if (value == 0) { // Reset.
+  ::wattr_set(m_window, saved_attr, saved_pair, &saved_opts);
+  if (blue)
+::wattron(m_window, COLOR_PAIR(16));
+} else {
+  // Only 8 basic foreground colors, our Highlighter doesn't use
+  // anything else.
+  assert(value >= 30 && value <= 37);
+  // Mapped directly to first 16 color pairs (black/blue background).
+  ::wattron(m_window, COLOR_PAIR(value - 30 + 1 + (blue ? 8 : 0)));
+}
+last_text = i;
+  }
+}
+if (text_length > 0)
+  PutCStringTruncated(right_pad, string.data() + last_text + 1,
+  text_length);
+::wattr_set(m_window, saved_attr, saved_pair, &saved_opts);
+  }
+
   void Touch() {
 ::touchwin(m_window);
 if (m_parent)
@@ -543,7 +598,7 @@
   void DrawTitleBox(const char *title, const char *bottom_message = nullptr) {
 attr_t attr = 0;
 if (IsActive())
-  attr = A_BOLD | COLOR_PAIR(2);
+  attr = A_BOLD | COLOR_PAIR(18);
 else
   attr = 0;
 if (attr)
@@ -944,9 +999,9 @@
   } else {
 const int shortcut_key = m_key_value;
 bool underlined_shortcut = false;
-const attr_t hilgight_attr = A_REVERSE;
+const attr_t highlight_attr = A_REVERSE;
 if (highlight)
-  window.AttributeOn(hilgight_attr);
+  window.AttributeOn(highlight_attr);
 if (llvm::isPrint(shortcut_key)) {
   size_t lower_pos = m_name.find(tolower(shortcut_key));
   size_t upper_pos = m_name.find(toupper(shortcut_key));
@@ -973,18 +1028,18 @@
 }
 
 if (highlight)
-  window.AttributeOff(hilgight_attr);
+  window.AttributeOff(highlight_attr);
 
 if (m_key_name.empty()) {
   if (!underlined_shortcut && llvm::isPrint(m_key_value)) {
-window.AttributeOn(COLOR_PAIR(3));
+window.AttributeOn(COLOR_PAIR(19));
 window.Printf(" (%c)", m_key_value);
-window.AttributeOff(COLOR_PAIR(3));
+window.AttributeOff(COLOR_PAIR(19));
   }
 } else {
-  window.AttributeOn(COLOR_PAIR(3));
+  window.AttributeOn(COLOR_PAIR(19));
   window.Printf(" (%s)", m_key_name.c_str());
-  window.AttributeOff(COLOR_PAIR(3));
+  window.AttributeOff(COLOR_PAIR(19));
 }
   }
 }
@@ -996,7 +1051,7 @@
   Menu::Type menu_type = GetType();
   switch (menu_type) {
   case Menu::Type::Bar: {
-window.SetBackground(2);
+windo