Re: [PATCH v2 2/9] gdb: introduce rgb_color type to simplify existing code

[Tom Tromey <tom@tromey.com>]

>>>>> Matthieu Longo <matthieu.longo@arm.com> writes:

> This patch replaces the raw uint8[3] buffer used to represent RGB values
> with a more convenient wrapper, rgb_color, around std::array<uint8_t, 3>.
> It also changes the return type of ui_file_style::color::get_rgb to
> rgb_color instead of filling a caller-provided buffer, and updates all
> callers accordingly.

Nice, thanks.

>    else if (m_color_space == color_space::ANSI_8COLOR
>  	   && 0 <= m_value && m_value <= 7)
> -    memcpy (rgb, palette_8colors[m_value], 3 * sizeof (uint8_t));
> +    memcpy (rgb, palette_8colors[m_value], rgb.size_bytes ());
>    else if (m_color_space == color_space::AIXTERM_16COLOR
>  	   && 0 <= m_value && m_value <= 15)
> -    memcpy (rgb, palette_16colors[m_value], 3 * sizeof (uint8_t));
> +    memcpy (rgb, palette_16colors[m_value], rgb.size_bytes ());
>    else if (m_color_space != color_space::XTERM_256COLOR)
>      gdb_assert_not_reached ("get_rgb called on invalid color");
>    else if (0 <= m_value && m_value <= 15)
> -    memcpy (rgb, palette_16colors[m_value], 3 * sizeof (uint8_t));
> +    memcpy (rgb, palette_16colors[m_value], rgb.size_bytes ());

I wonder if the types of palette_8colors and palette_16colors should
change as well.  Then it seems the memcpy could be an assignment.

> diff --git a/gdb/ui-style.h b/gdb/ui-style.h
> index fca9150889b..72349256034 100644
> --- a/gdb/ui-style.h
> +++ b/gdb/ui-style.h
> @@ -54,6 +54,34 @@ extern bool color_space_safe_cast (color_space *result, long c);
>  /* Get the number of colors supported by the terminal where GDB is running.  */
>  extern int gdb_get_ncolors ();
 
> +struct rgb_color
> +{

Usually in gdb we add a comment before a struct definition that explains
what it is about.

> +  constexpr const uint8_t& operator[](std::size_t idx) const noexcept
> +  { return m_data[idx]; }

The "&" should come after the space here.
Also a space between "[]" and "()".

> +    color (const rgb_color &rgb)
> +      : m_color_space (color_space::RGB_24BIT),
> +	m_red (rgb.r ()),
> +	m_green (rgb.g ()),
> +	m_blue (rgb.b ())
> +    {
> +    }

Probably this constructor should be 'explicit'.  That's the norm in gdb
unless there's a real need for an implicit conversion.

thanks,
Tom