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

Matthieu Longo
2026-03-04T16:36:50Z

On 03/03/2026 18:16, Tom Tromey wrote:
>>>>>> 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.
>

Yes, it makes sense. I hadn't looked too much at the code using it.
Ok, I will fix this in the new revision.

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

/* Convenient wrapper for RGB color values. */ ?

>> + 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 "()".
>

Fixed.

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

Fixed.

>
> thanks,
> Tom

Matthieu