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

From: Jeff King <peff@peff.net>
Date: Nov 20, 2014, 20:10 UTC
Message-ID: <20141120201026.GA4779@peff.net>
In-Reply-To: <xmqqh9xt3bdx.fsf@gitster.dls.corp.google.com>

On Thu, Nov 20, 2014 at 11:44:26AM -0800, Junio C Hamano wrote:

Show 16 quoted lines

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

-Peff