This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 03/16] Introduce ui_file_style
- From: Joel Brobecker <brobecker at adacore dot com>
- To: Tom Tromey <tom at tromey dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Mon, 24 Dec 2018 07:40:18 +0400
- Subject: Re: [PATCH 03/16] Introduce ui_file_style
- References: <20181128001435.12703-1-tom@tromey.com> <20181128001435.12703-4-tom@tromey.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