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

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

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 ?

Matthieu