Re: [PATCH 4/7] parse_color: refactor color storage

[Johannes Sixt <j6t@kdbg.org>]

Am 09.12.2014 um 21:21 schrieb Jeff King:
> On Tue, Dec 09, 2014 at 09:14:29PM +0100, Johannes Sixt wrote:
> 
>> Am 20.11.2014 um 16:17 schrieb Jeff King:
>>> +#define COLOR_FOREGROUND '3'
>>> +#define COLOR_BACKGROUND '4'
>>
>> This (COLOR_BACKGROUND) causes an ugly redefinition warning on Windows,
>> because we inherit a definition from a Windows header. How would you
>> like it fixed? A different name or #undef in front of it?
> 
> I think a different name would be fine. The constants are actually only
> used once each. Their main function is to avoid a confusing parameter
> '3' to the color_output function. But we could use the literal
> constants with a parameter, like:
> 
> diff --git a/color.c b/color.c
> index e2a0a99..809b359 100644
> --- a/color.c
> +++ b/color.c
> @@ -144,9 +144,6 @@ int color_parse(const char *value, char *dst)
>  	return color_parse_mem(value, strlen(value), dst);
>  }
>  
> -#define COLOR_FOREGROUND '3'
> -#define COLOR_BACKGROUND '4'
> -
>  /*
>   * Write the ANSI color codes for "c" to "out"; the string should
>   * already have the ANSI escape code in it. "out" should have enough
> @@ -245,12 +242,14 @@ int color_parse_mem(const char *value, int value_len, char *dst)
>  		if (!color_empty(&fg)) {
>  			if (sep++)
>  				*dst++ = ';';
> -			dst = color_output(dst, &fg, COLOR_FOREGROUND);
> +			/* foreground colors are all in the 3x range */
> +			dst = color_output(dst, &fg, '3');
>  		}
>  		if (!color_empty(&bg)) {
>  			if (sep++)
>  				*dst++ = ';';
> -			dst = color_output(dst, &bg, COLOR_BACKGROUND);
> +			/* background colors are all in the 4x range */
> +			dst = color_output(dst, &bg, '4');
>  		}
>  		*dst++ = 'm';
>  	}
> 
> We could also pass in integer "30" and "40", and then format it with %d
> inside color_output. I don't know if that would be more obvious or not.

This patch would actually be my personal preference. The comment near
the literal makes it all clear.

-- Hannes