Re: [PATCH 5/7] parse_color: support 24-bit RGB values

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 20, 2014, 20:25 UTC
Message-ID: <xmqq8uj539hi.fsf@gitster.dls.corp.google.com>
In-Reply-To: <20141120201026.GA4779@peff.net>

Jeff King <peff@peff.net> writes:

Show 29 quoted lines

> On Thu, Nov 20, 2014 at 11:44:26AM -0800, Junio C Hamano wrote:
>
>> > @@ -32,10 +32,13 @@ struct color {
>> >  		COLOR_UNSPECIFIED = 0,
>> >  		COLOR_NORMAL,
>> >  		COLOR_ANSI, /* basic 0-7 ANSI colors */
>> > -		COLOR_256
>> > +		COLOR_256,
>> > +		COLOR_RGB
>> >  	} state;
>> >  	/* The numeric value for ANSI and 256-color modes */
>> >  	unsigned char value;
>> > +	/* 24-bit RGB color values */
>> > +	unsigned char red, green, blue;
>> 
>> Do value and rgb have to be both valid at the same time, or is this
>> "we are not wasting a byte by not using a union because it will be
>> in the padding of the outer struct anyway"?
>
> The latter. I started with a union, and then realized that COLOR_ANSI
> and COLOR_256 shared the value, so the union was not saving space and
> just getting in the way (mostly because I had to think of useful names
> for each of the members).
>
> I'd be happy to do it as a union if you think that makes it clearer.
>
> Also, the name "state" should perhaps be "type". It originally
> started as "unspecified or an actual value", which is a state, but
> as I worked, it grew into something more.

I think use of union might be more "kosher", e.g.

	struct color_spec {
        	enum { ... } type;
                union {
                	struct { unsigned char r, g, b; } rgb;
                        unsigned char ansi;
		} u;
	} c;

but it is not like you have an array of these things for each slot, and with the intervening ".u.<type>" you have to write every time you refer to these fields, the result is probably much uglier and harder to read. So let's only do s/state/type/ and leave these "ought to be union but that will be uglier" ones as they are.