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

From: Jeff King <peff@peff.net>
Date: Dec 9, 2014, 20:21 UTC
Message-ID: <20141209202135.GB12001@peff.net>
In-Reply-To: <54875825.7060008@kdbg.org>

On Tue, Dec 09, 2014 at 09:14:29PM +0100, Johannes Sixt wrote:

Show 7 quoted lines

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

-Peff