> 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 _______________________________________________________________________