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

[Matthieu Longo <matthieu.longo@arm.com>]

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