Re: [dev] [st] [patch] Cleanup config.def.h, eliminate mappedkeys, simplify matching.

From: Alexander S. <alex0player_AT_gmail.com>
Date: Sat, 19 Oct 2013 14:43:19 +0400

2013/10/19 Mark Edgar <medgar123_AT_gmail.com>:
> A series of patches for consideration.
>
> The first patch is purely aesthetic: it cleans up column headings (comments)
> and internal tabs in the shortcuts/key/mshortcuts tables in config.def.h. It
> also renames the fields in Key to match the nicer names given in
> config.def.h.
>
> The second patch removes the mappedkeys[] optimization. I tested this by
> adding 1.000.000 additional entries to the end of key[]:
>
> static Key key[] = {
> ...
> #include "lots"
> /*
> * "lots" contains 999.999 repetitions of this entry, which is crafted
> * to hit all the tests in kmap() and fail the last:
> * { 'x', XK_ANY_MOD, "nope", .keypad=-1, .cursor=-1, .crlf=1 },
> */
> { 'x', XK_NO_MOD, "exit" },
> };
>
> While gcc goes from a few seconds to more than a minute to compile and link
> the above, I could not detect a performance regression in st. ;)
>
> The third patch simplifies the matching logic in kmap() and match() without
> changing the behavior. Hopefully, it is straightforward, but it probably
> deserves a careful reading to make sure I haven't made any mistakes.
Why removing mappedkeys? Sure, there may be no visible regression, but
checking every keypress against key bindings seems unreasonable to me.
As for the last patch: it seems to me that behaviour is unchanged.
However, I do not like using != as a logical operator. I'd suggest
using (IS_SET(MODE_APPKEYPAD) ? kp->keypad < 0 : kp->keypad > 0),
which is about the same length, but a bit more clear. I'd also suggest
following the same pattern for all three clauses, as nested if() is
unnecessary (clearly, kp->keypad == 2 may evaluate to true only if
kp->keypad is nonzero).
Received on Sat Oct 19 2013 - 12:43:19 CEST