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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
