Re: [PATCH 03/16] Introduce ui_file_style

Joel Brobecker
2018-12-24T03:40:28Z

On Tue, Nov 27, 2018 at 05:14:22PM -0700, Tom Tromey wrote:
> This introduces the new ui_file_style class and various helpers. This
> class represents a terminal style and provides methods for parsing and
> emitting the corresponding ANSI terminal escape sequences.
>
> gdb/ChangeLog
> 2018-11-27 Tom Tromey <tom@tromey.com>
>
> * unittests/style-selftests.c: New file.
> * ui-style.c: New file.
> * ui-style.h: New file.
> * ui-file.h: Include ui-style.h.
> * Makefile.in (COMMON_SFILES): Add ui-style.c.
> (HFILES_NO_SRCDIR): Add ui-style.h.
> (SUBDIR_UNITTESTS_SRCS): Add style-selftests.c.

FWIW, this looks good overall. I have one minor comment, and
a question or two...

Having the unit tests is absolutely awesome!

> +/* See ui-style.h. */
> +
> +void
> +ui_file_style::color::get_rgb (uint8_t *rgb) const
> +{
> + if (m_simple)
> + {
> + /* Can't call this for a basic color. */

I am trying to understand this comment, as well as the corresponding
comment describing this method (in ui-style.h):

+ This may not be called for simple colors or for the "NONE"
+ color. */

Does it look like you can, in fact, call this method on
a simple color?

> + ui_file_style ()
> + {
> + }

Sorry for the obvious C++ question - Does this constructor means
that the default constructor results in an object with undefined
data? Or is that the same as...

ui_file_style () = default;

It seems that it would be the latter case, but then I don't see
why we would want that...

> +/* Skip an ANSI escape sequence in BUF. BUF must begin with an ESC
> + character. Return true if an escape sequence was successfully
> + skipped; false otherwise. In either case, N_READ is updated to
> + reflect the number of chars read from BUF. */
> +
> +extern bool skip_ansi_escape (const char *buf, int *bytes);

I think there might be a copy-pasto in the comment: N_READ -> BYTES?

--
Joel