> 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.
Please send atomic patches, The patch must do only one thing and
only one. In this case the tab part is not correct, because we use
smart tabs in the project, tabs for indentation and spaces for
alignment. About the name of the fields, I think is a good idea
have the same in both places, but I am not sure where we have to
change the name. appkey and appcursor have the 'app' prefix because
they are related to the application modes of the terminal, so moving
them to key and cursor quits also this information, but it also
reduce the length of the name, which is good.
>
> 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. ;)
I have send the reply to this point in the other mail.
>
> 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.
>
Uysss, here be dragons. These conditions were a point of problems
in the past, and the actual result, is a collection of different
patches to fix different problems. So any change will be tested in
deep before applying it.
> static inline bool
> match(uint mask, uint state) {
> state &= ~ignoremod;
>-
>- if(mask == XK_NO_MOD && state)
>- return false;
>- if(mask != XK_ANY_MOD && mask != XK_NO_MOD && !state)
>- return false;
>- if(mask == XK_ANY_MOD)
>- return true;
>- return state == mask;
>+ return mask == XK_ANY_MOD || state == mask;
> }
>
>
I think this patch is correct, because XK_NO_MOD is 0, so if there is
some modifier then 'state == mask' will be false. After the simplification,
match is too short and does nothing, so I think it should be
merged into kmap.
>@@ -3528,25 +3521,18 @@ kmap(KeySym k, uint state) {
> if(!match(kp->mask, state))
> continue;
>
>- if(kp->keypad > 0) {
>- if(!IS_SET(MODE_APPKEYPAD))
>+ if(kp->keypad) {
>+ if(IS_SET(MODE_APPKEYPAD) != (kp->keypad > 0))
> continue;
> if(term.numlock && kp->keypad == 2)
> continue;
>- } else if(kp->keypad < 0 && IS_SET(MODE_APPKEYPAD)) {
>- continue;
> }
>
>- if((kp->cursor < 0 && IS_SET(MODE_APPCURSOR)) ||
>- (kp->cursor > 0
>- && !IS_SET(MODE_APPCURSOR))) {
>+ if(kp->cursor && IS_SET(MODE_APPCURSOR) != (kp->cursor > 0))
> continue;
>- }
>
>- if((kp->crlf < 0 && IS_SET(MODE_CRLF)) ||
>- (kp->crlf > 0 && !IS_SET(MODE_CRLF))) {
>+ if(kp->crlf && IS_SET(MODE_CRLF) != (kp->crlf > 0))
> continue;
>- }
>
> return kp->s;
> }
>--
I agree with Alexander here, and maybe the ?: version could be better.
--
Roberto E. Vargas Caballero
_______________________________________________________________________
'Write programs that do one thing and do it well. Write programs to work
together. Write programs to handle text streams, because that is a
universal interface'
(Doug McIlroy)
In Other Words - Don't design like polkit or systemd
_______________________________________________________________________
