So I have a confession. I had not set up my git clone of awesome properly and I did not know how to specify the INSTALL_PREFIX and SYSCONFDIR variables correctly so I was trying to run awesome from the build dir in the clone repo whenever I needed to test patches. Often times I was not able to, and when I had a patch that I thought was so simple it didn't need testing I just asked for it to be accepted.
Like the 3 I had attached in my previous message. The only damn problem was the 0001-Use-RGB_16TO8-in-luaA_push_color.patch I wrote a zero instead of an 'O' in one of the RGB_ macro names. So I remade the patch instead of just editing the patch directly. Problem is, I combined the last 2 of those 3 patches because I didn't feel like doing this incrementally. So for the last time (hopefully), here are the patches in the order they should be applied: 1) 0001-Deduplicate-the-warn-in-in-color_init_unchecked.patch 2) 0001-Refine-RGB_-macros-use-RGB_16TO8-in-luaA_push_color.patch psychon please just accept them, I think I'm going crazy. And this all was so simple, I should not have failed so many times. And now my fail will be immortalized in mailinglist history :( The 2 attached patches on this last message are the final and correct ones. And this time I compiled and ran awesome to test them. And that shall be what I do from now on. EVEN IF I CHANGE ONLY COMMENTS >8| On Fri, Nov 18, 2011 at 1:06 PM, Majic <[email protected]> wrote: > 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 d1aac73bcb3dd8c3636d3c7895301f6270d31ce5 Mon Sep 17 00:00:00 2001 From: Majic <[email protected]> Date: Fri, 18 Nov 2011 23:56:29 -0800 Subject: [PATCH] Refine RGB_ macros, use RGB_16TO8() in luaA_push_color() --- color.c | 20 ++++++++++++-------- 1 files changed, 12 insertions(+), 8 deletions(-) diff --git a/color.c b/color.c index faba448..8d93a14 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 (257) */ +#define RGB_8TO16(i) (((i) & 0xff) * 0x101) +#define RGB_16TO8(i) (((i) & 0xffff) / 0x101) /** Parse an hexadecimal color string to its component. * \param colstr The color string. @@ -45,14 +46,16 @@ 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; *blue = colnum & 0xff; return true; @@ -128,9 +131,10 @@ 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
