Re: [PATCH 03/16] Introduce ui_file_style

[Joel Brobecker <brobecker@adacore.com>]

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