Hmm so, I took my own advice and psychon's and split it into 3 patches, with the last being kind of unnecessary but I'd love to see it accepted anyway :>
In attached patch application order: 1) 0001-Deduplicate-the-warn-in-in-color_init_unchecked.patch This patch gets rid of the same warn() message being sent in color_init_unchecked(). The problem is color_parse() sends the same message and unchecked() sends if color_parse() fails. The duplicated message is somewhat unhelpful. 2) 0001-Use-RGB_16TO8-in-luaA_push_color.patch This patch makes use of RGB_16TO8() in luaA_push_color() which was defined but not used. I also attempted to clean up how the RGB_16TO8() and RGB_8TO16() macros were written. The [good] compiler would of course equate 0xFFFF / 0xFF to 0x101 as a constant expression but I thought it was clearer written my way with a comment to explain what 0x101 is. I think I'll also keep a lookout for anywhere else in awesome that might have missed the use of these RGB_ macros. The main thing is the change to luaA_push_color() relying on the macros rather than duplicated code. 3) 0001-Imperceptable-optimization-to-color_parse.patch This patch is unnecessary but I like how I wrote it rather than the original. :> I made a minor change to the if statement that warn()'s on an invalid color so it *might* short-circuit earlier and then I also rewrote the section where the first 3 octets of colnum were split into their own uint8_t red, green, and blue values. I know *red = (colnum >> 16) & 0xff; is pretty clear already but if it were up to me I would shift it over octet by octet to make it transparent to the skimming reader than the colors are similarly being popped off of an array. The first time I saw the (colnum >> 16) portion I wasn't quite sure where red was, it being the last octet masked off of colnum seems clearer to me that it is the third octet being harvested outright. Furthermore (colnum >> 16) on a good but not perfect compiler creates a temporary, (colnum >> 8) does also. My way reuses colnum which is hopefully already in a register. colnum isn't used after that at all so the modification of colnum is perfectly fine for its current use. :> I have been told by a friend that a *great* compiler would notice that colnum isn't used at all after this and would instead modify colnum without creating temporaries from it. But I don't trust compilers to be that omnicient. Still, if this means nothing then I'm disappointed but I won't start a holy war if this patch isn't accepted. Isn't it just ever so much fun to obsess about the little things? :p Toodles :] Patch order: 1) 0001-Deduplicate-the-warn-in-in-color_init_unchecked.patch 2) 0001-Use-RGB_16TO8-in-luaA_push_color.patch 3) 0001-Imperceptable-optimization-to-color_parse.patch On Fri, Nov 18, 2011 at 10:37 AM, Majic <[email protected]> wrote: > So I have a few minor changes to color.c :> > > I actually posted this linked from sprunge.us a couple times in > #awesome and it wasn't correct all those times. So this is meant to > be the final patch where everything is good and happy. :x > > See attached. > > Would love some peer review and maybe eventually accepted into master? > The only validly good thing about this patch is it removes a problem > where if color_parse() failed it would warn about the color being > invalid twice. (Since the warn() was sent again after the return of > color_parse() in color_init_unchecked() Maybe this should be two > patches, or three, but I felt that all of it was simple enough to be > one :> If it were 3 it would be "deduplicate warn() used in > color_parse() and color_init_unchecked()", make the bit harvesting in > color_parse() (possibly?) more understandable and combine the if's in > color_init_unchecked()", then "rework the RGB_8TO16/16TO8 macros and > use 16TO8 in luaA_push_color()" But that's like 3 patches for pretty > simple changes. :> > > The important thing to remember is 0xFFFF / 0xFF == 0x101 >.< > > Toodles. > > PS: The only reason I touched the if in color_parse() was to help it > short-circuit sooner. >
From 552ee90b1fcb38bff3c0313e689015c91f1fadf2 Mon Sep 17 00:00:00 2001 From: Majic <[email protected]> Date: Fri, 18 Nov 2011 20:36:34 -0800 Subject: [PATCH] Deduplicate the warn() in in color_init_unchecked() --- color.c | 18 ++++-------------- 1 files changed, 4 insertions(+), 14 deletions(-) diff --git a/color.c b/color.c index ceee8d4..faba448 100644 --- a/color.c +++ b/color.c @@ -74,31 +74,21 @@ color_init_unchecked(color_t *color, const char *colstr, ssize_t len) p_clear(&req, 1); - if(!len) - { - req.has_error = true; - return req; - } - - req.color = color; - /* The color is given in RGB value */ - if(!color_parse(colstr, len, &red, &green, &blue)) + if(len == 0 || !color_parse(colstr, len, &red, &green, &blue)) { - warn("awesome: error, invalid color '%s'", colstr); req.has_error = true; return req; } + req.color = color; + req.has_error = false; + req.colstr = colstr; req.cookie_hexa = xcb_alloc_color_unchecked(globalconf.connection, globalconf.default_cmap, RGB_8TO16(red), RGB_8TO16(green), RGB_8TO16(blue)); - - req.has_error = false; - req.colstr = colstr; - return req; } -- 1.7.7.3
From d68e17e5c11255ff9d09d79bddee6839a1313b25 Mon Sep 17 00:00:00 2001 From: Majic <[email protected]> Date: Fri, 18 Nov 2011 20:46:39 -0800 Subject: [PATCH] Use RGB_16TO8() in luaA_push_color() --- color.c | 11 ++++++----- 1 files changed, 6 insertions(+), 5 deletions(-) diff --git a/color.c b/color.c index faba448..08df0dc 100644 --- a/color.c +++ b/color.c @@ -26,8 +26,9 @@ #include "globalconf.h" #include "common/xutil.h" -#define RGB_8TO16(i) (0xffff * ((i) & 0xff) / 0xff) -#define RGB_16TO8(i) (0xff * ((i) & 0xffff) / 0xffff) +/* 0xffff / 0xff == 0x101 */ +#define RGB_8T016(i) (((i) & 0xff) * 0x101) +#define RGB_16TO8(i) (((i) & 0xffff) / 0x101) /** Parse an hexadecimal color string to its component. * \param colstr The color string. @@ -128,9 +129,9 @@ color_init_reply(color_init_request_t req) int luaA_pushcolor(lua_State *L, const color_t c) { - uint8_t r = (unsigned) c.red * 0xff / 0xffff; - uint8_t g = (unsigned) c.green * 0xff / 0xffff; - uint8_t b = (unsigned) c.blue * 0xff / 0xffff; + uint8_t r = RGB_16TO8(c.red); + uint8_t g = RGB_16TO8(c.green); + uint8_t b = RGB_16TO8(c.blue); char s[10]; int len = snprintf(s, sizeof(s), "#%02x%02x%02x", r, g, b); lua_pushlstring(L, s, len); -- 1.7.7.3
From b4179f9493b08ff87082daaa22aa88ad8d50d221 Mon Sep 17 00:00:00 2001 From: Majic <[email protected]> Date: Fri, 18 Nov 2011 20:50:20 -0800 Subject: [PATCH] Imperceptable optimization to color_parse() --- color.c | 8 +++++--- 1 files changed, 5 insertions(+), 3 deletions(-) diff --git a/color.c b/color.c index 08df0dc..cc363c1 100644 --- a/color.c +++ b/color.c @@ -46,15 +46,17 @@ color_parse(const char *colstr, ssize_t len, char *p; colnum = strtoul(colstr + 1, &p, 16); - if(len != 7 || (p - colstr) != 7 || colstr[0] != '#') + if(len != 7 || colstr[0] != '#' || (p - colstr) != 7) { warn("awesome: error, invalid color '%s'", colstr); return false; } - *red = (colnum >> 16) & 0xff; - *green = (colnum >> 8) & 0xff; *blue = colnum & 0xff; + colnum >>= 8; + *green = colnum & 0xff; + colnum >>= 8; + *red = colnum & 0xff; return true; } -- 1.7.7.3
