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

Tom Tromey
2026-03-03T18:17:18Z

>>>>> 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