Re: [dev] [st] [PATCH] Refactor xsetcolorname()

From: FRIGN <dev_AT_frign.de>
Date: Mon, 26 May 2014 15:18:48 +0200

On Mon, 26 May 2014 08:56:58 +0400
"Alexander S." <alex0player_AT_gmail.com> wrote:

Hey Alexander,

thanks for your feedback!

> this function implements STR command for changing a palette. Empty
> name specifically tells to revert to system color, thus, if(!name)
> clause is there for restoring a color that may have been changed using
> the same procedure.

I found out later; see my other replies. Thanks for clearing that up
again, though.

> Here:
> > if(BETWEEN(x, 16, 6*6*6+16)) { /* 256 colour */
> you forgot -1. Don't forget BETWEEN() is inclusive on both ends.
> However, the removal of extra stack variable and some additional
> comments are very nice. I wholeheartedly approve of them.

Good catch! I'll put that into the updated version of this patch.

> Your concern about repetitiveness is valid and bothered me when I
> wrote that function, although back then xloadcols() was in much poorer
> state. By now, we may as well reimplement xloadcols() in terms of
> xsetcolorname(i, NULL). I'd be glad if you did this, as unfortunately,
> I do not have access to development environment right now.

I'd be happy to do this! This patch alone makes the binary ~0.5KB
smaller. Let's see how much this additional cut will bring!

Cheers

FRIGN

-- 
FRIGN <dev_AT_frign.de>
Received on Mon May 26 2014 - 15:18:48 CEST

This archive was generated by hypermail 2.3.0 : Mon May 26 2014 - 15:24:06 CEST