Re: [PATCH v3 1/7] gdb: introduce rgb_color type to simplify existing code

[Simon Marchi <simark@simark.ca>]

On 3/11/26 1:55 PM, Matthieu Longo wrote:
> On 11/03/2026 15:03, Simon Marchi wrote:
>> On 3/9/26 3:22 PM, 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.
>>>
>>> One tiny nit.
>>>
>>>>     if (target_space == color_space::RGB_24BIT)
>>>>       {
>>>> -      uint8_t rgb[3];
>>>> -      get_rgb (rgb);
>>>> -      return color (rgb[0], rgb[1], rgb[2]);
>>>> +      return color (get_rgb ());
>>>>       }
>>>   Since there's only one statement now, the braces should be removed.
>>>
>>> This is ok with this change.
>>> Approved-By: Tom Tromey <tom@tromey.com>
>>>
>>> Tom
>>
>> This appears to have caused a build failure on armhf:
>>
>> https://builder.sourceware.org/buildbot/#/builders/72/builds/7402/steps/4/logs/stdio
>>
>> Sample:
>>
>> ../../binutils-gdb/gdb/tui/tui-io.c: In function ‘bool get_color(const ui_file_style::color&, int*)’:
>> ../../binutils-gdb/gdb/tui/tui-io.c:260:38: error: ISO C++ says that these are ambiguous, even though the worst conversion for the first is better than the worst conversion for the second: [-Werror]
>>    260 |           if (init_color (next, rgb[0] * 1000 / 255, rgb[1] * 1000 / 255,
>>        |                                      ^
>> In file included from ./../../binutils-gdb/gdb/ui-file.h:23,
>>                   from ./../../binutils-gdb/gdb/defs.h:60,
>>                   from <command-line>:
>> ./../../binutils-gdb/gdb/ui-style.h:78:22: note: candidate 1: ‘constexpr uint8_t& rgb_color::operator[](std::size_t)’
>>     78 |   constexpr uint8_t &operator[] (std::size_t idx) noexcept
>>        |                      ^~~~~~~~
>> ../../binutils-gdb/gdb/tui/tui-io.c:260:38: note: candidate 2: ‘operator[](uint8_t* {aka unsigned char*}, int)’ (built-in)
>>    260 |           if (init_color (next, rgb[0] * 1000 / 255, rgb[1] * 1000 / 255,
>>        |                                      ^
>>
>> It seems to be because an rgb_color can be implicitly converted to a
>> `uint8_t *`, which can be subscripted, but also has an operator[].
>>
>> Not sure why the uint8_t* operator is needed,
> 
> It was originally required for this line when passing rgb to memcpy().
>  
>   memcpy (rgb, palette_8colors[m_value], rgb.bytes_size ());
> 
> But was later replaced by this assignment:
> 
>   rgb = palette_8colors[m_value];
> 
>> but it compiles fine here if I remove it.  If really needed, I think that an explicit method
>> (`.data()`) would be preferable, to avoid unwanted conversions.
>>
>> Simon
> 
> I locally tested the removal of this uint8_t* operator, and the code compiles fine.
> 
> diff --git a/gdb/ui-style.h b/gdb/ui-style.h
> index a1d656cd39c..fc40b93709d 100644
> --- a/gdb/ui-style.h
> +++ b/gdb/ui-style.h
> @@ -72,7 +72,6 @@ struct rgb_color
>    constexpr uint8_t g () const noexcept { return m_data[1]; }
>    constexpr uint8_t b () const noexcept { return m_data[2]; }
> 
> -  constexpr operator uint8_t *() noexcept { return m_data.data (); }
>    constexpr size_t size () const noexcept { return m_data.size (); }
> 
>    constexpr uint8_t &operator[] (std::size_t idx) noexcept
> 
> Do you want me to publish a patch for this ? Or will you fix it ?

Please push a patch that does exactly that.  You can put my:

Approved-By: Simon Marchi <simon.marchi@efficios.com>

Simon