makekeys: replace helper with python script and binary search

This removes the complicated and undocumented hash-table creation-helper
and replaces it with an autogenerated sorted array. The search uses simple
bsearch() now.

We also tried using gperf but it turned out to generate way to big
hashtables and when reducing the size it isn't really faster than
bsearch() anymore.

There are no users complaining about the speed of keysym lookups and we
have no benchmarks that tell that we are horribly slow. Hence, we can
safely use the simpler approach and drop all that old code.

Signed-off-by: Ran Benita <ran234@gmail.com>
Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>

Makefile.am

@@ -1,7 +1,5 @@
 ACLOCAL_AMFLAGS = -I m4
 
-SUBDIRS = makekeys
-
 pkgconfigdir = $(libdir)/pkgconfig
 pkgconfig_DATA = xkbcommon.pc
 
@@ -92,11 +90,8 @@ src/xkbcomp/parser.c: $(top_builddir)/src/$(am__dirstamp) $(top_builddir)/src/xk
 src/xkbcomp/parser.h: $(top_builddir)/src/$(am__dirstamp) $(top_builddir)/src/xkbcomp/$(am__dirstamp)
 src/xkbcomp/scanner.c: $(top_builddir)/src/$(am__dirstamp) $(top_builddir)/src/xkbcomp/$(am__dirstamp)
 
-src/ks_tables.h: $(top_builddir)/makekeys/makekeys$(EXEEXT)
+src/ks_tables.h: makekeys.py
-	$(AM_V_GEN)$(top_builddir)/makekeys/makekeys $(top_srcdir)/xkbcommon/xkbcommon-keysyms.h > $@
+	$(AM_V_GEN)$(PYTHON) $(top_srcdir)/makekeys.py $(top_srcdir)/xkbcommon/xkbcommon-keysyms.h > $@
-
-$(top_builddir)/makekeys/makekeys$(EXEEXT): $(top_srcdir)/makekeys/makekeys.c
-	$(MAKE) -C makekeys
 
 # Documentation
 

configure.ac

@@ -61,6 +61,9 @@ if test ! -f "src/xkbcomp/parser.c"; then
       AC_MSG_ERROR([yacc not found - unable to compile src/xkbcomp/parser.y])
    fi
 fi
+AM_PATH_PYTHON([2.6], [], [
+    AC_MSG_ERROR([python not found - unable to run makekeys])
+])
 
 # Checks for library functions.
 AC_CHECK_FUNCS([strcasecmp strncasecmp])
@@ -71,16 +74,6 @@ fi
 
 AC_CHECK_FUNCS([eaccess euidaccess])
 
-# Build native compiler needed for makekeys
-AC_ARG_VAR([CC_FOR_BUILD], [Build native C compiler program])
-if test "x$CC_FOR_BUILD" = x; then
-    if test "$cross_compiling" != no; then
-        AC_PATH_PROGS([CC_FOR_BUILD], [gcc cc], [cc])
-    else
-        CC_FOR_BUILD="$CC"
-    fi
-fi
-
 XORG_TESTSET_CFLAG([CFLAGS], [-fvisibility=hidden])
 
 # Define a configuration option for the XKB config root
@@ -121,7 +114,6 @@ AC_DEFINE_UNQUOTED([DEFAULT_XKB_LAYOUT], ["$DEFAULT_XKB_LAYOUT"],
 
 AC_CONFIG_FILES([
     Makefile
-    makekeys/Makefile
     xkbcommon-uninstalled.pc
     xkbcommon.pc
     doc/Doxyfile

makekeys.py

@@ -0,0 +1,23 @@
+#!/usr/bin/env python
+
+import re, sys, itertools
+
+pattern = re.compile(r'^#define\s+XKB_KEY_(?P<name>\w+)\s+(?P<value>0x[0-9a-fA-F]+)\s')
+matches = [pattern.match(line) for line in open(sys.argv[1])]
+entries = [(m.group("name"), int(m.group("value"), 16)) for m in matches if m]
+
+print('''struct name_keysym {
+    const char *name;
+    xkb_keysym_t keysym;
+};\n''')
+
+print('static const struct name_keysym name_to_keysym[] = {');
+for (name, _) in sorted(entries, key=lambda e: e[0]):
+    print('    {{ "{name}", XKB_KEY_{name} }},'.format(name=name))
+print('};\n')
+
+# *.sort() is stable so we always get the first keysym for duplicate
+print('static const struct name_keysym keysym_to_name[] = {');
+for (name, _) in (next(g[1]) for g in itertools.groupby(sorted(entries, key=lambda e: e[1]), key=lambda e: e[1])):
+    print('    {{ "{name}", XKB_KEY_{name} }},'.format(name=name))
+print('};')

makekeys/.gitignore

@@ -1 +0,0 @@
-makekeys

makekeys/Makefile.am

@@ -1,10 +0,0 @@
-AM_CFLAGS = $(BASE_CFLAGS) -I$(top_srcdir)
-
-# need to use build-native compiler
-
-CC = $(CC_FOR_BUILD)
-CPPFLAGS = $(CPPFLAGS_FOR_BUILD)
-CFLAGS = $(CFLAGS_FOR_BUILD)
-LDFLAGS = $(LDFLAGS_FOR_BUILD)
-noinst_PROGRAMS = makekeys
-

makekeys/makekeys.c

@@ -1,302 +0,0 @@
-/*
- *
- * Copyright 1990, 1998  The Open Group
- *
- * Permission to use, copy, modify, distribute, and sell this software and its
- * documentation for any purpose is hereby granted without fee, provided that
- * the above copyright notice appear in all copies and that both that
- * copyright notice and this permission notice appear in supporting
- * documentation.
- *
- * The above copyright notice and this permission notice shall be included
- * in all copies or substantial portions of the Software.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
- * OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
- * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.
- * IN NO EVENT SHALL THE OPEN GROUP BE LIABLE FOR ANY CLAIM, DAMAGES OR
- * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
- * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
- * OTHER DEALINGS IN THE SOFTWARE.
- *
- * Except as contained in this notice, the name of The Open Group shall
- * not be used in advertising or otherwise to promote the sale, use or
- * other dealings in this Software without prior written authorization
- * from The Open Group.
- *
- */
-
-/*
- * Constructs hash tables for xkb_keysym_to_string and
- * xkb_string_from_keysym.
- */
-
-#include "xkbcommon/xkbcommon.h"
-
-#include <inttypes.h>
-#include <stdio.h>
-#include <stdlib.h>
-#include <string.h>
-
-typedef uint32_t Signature;
-
-#define KTNUM 4000
-
-static struct info {
-    char        *name;
-    xkb_keysym_t val;
-} info[KTNUM];
-
-#define MIN_REHASH 15
-#define MATCHES    10
-
-static char tab[KTNUM];
-static unsigned short offsets[KTNUM];
-static unsigned short indexes[KTNUM];
-static xkb_keysym_t values[KTNUM];
-static int ksnum = 0;
-
-static int
-parse_line(const char *buf, char *key, xkb_keysym_t *val, char *prefix)
-{
-    int i;
-    char alias[128];
-
-    /* See if we can catch a straight XK_foo 0x1234-style definition first;
-     * the trickery around tmp is to account for prefices. */
-    i = sscanf(buf, "#define %127s 0x%" SCNx32, key, val);
-    if (i == 2 && strncmp(key, "XKB_KEY_", 8) == 0) {
-        prefix[0] = '\0';
-        memmove(key, key + 8, strlen(key + 8) + 1);
-        return 1;
-    }
-
-    i = sscanf(buf, "#define %127s %127s", key, alias);
-    if (i == 2)
-        fprintf(stderr, "can't parse keysym definition: %s", buf);
-
-    return 0;
-}
-
-int
-main(int argc, char *argv[])
-{
-    FILE *fptr;
-    int max_rehash;
-    Signature sig;
-    int i, j, k, l, z;
-    char *name;
-    char c;
-    int first;
-    int best_max_rehash;
-    int best_z = 0;
-    int num_found;
-    xkb_keysym_t val;
-    char key[128], prefix[128];
-    char buf[1024];
-
-    for (l = 1; l < argc; l++) {
-        fptr = fopen(argv[l], "r");
-        if (!fptr) {
-            fprintf(stderr, "couldn't open %s\n", argv[l]);
-            continue;
-        }
-
-        while (fgets(buf, sizeof(buf), fptr)) {
-            if (!parse_line(buf, key, &val, prefix))
-                continue;
-
-            if (val == XKB_KEY_VoidSymbol)
-                val = 0;
-            if (val > 0x1fffffff) {
-                fprintf(stderr, "ignoring illegal keysym (%s, %" PRIx32 ")\n",
-                        key,
-                        val);
-                continue;
-            }
-
-            name = malloc(strlen(prefix) + strlen(key) + 1);
-            if (!name) {
-                fprintf(stderr, "makekeys: out of memory!\n");
-                exit(1);
-            }
-            sprintf(name, "%s%s", prefix, key);
-            info[ksnum].name = name;
-            info[ksnum].val = val;
-            ksnum++;
-            if (ksnum == KTNUM) {
-                fprintf(stderr, "makekeys: too many keysyms!\n");
-                exit(1);
-            }
-        }
-
-        fclose(fptr);
-    }
-
-    printf("/* This file is generated from keysymdef.h. */\n");
-    printf("/* Do not edit. */\n");
-    printf("\n");
-
-    best_max_rehash = ksnum;
-    num_found = 0;
-    for (z = ksnum; z < KTNUM; z++) {
-        max_rehash = 0;
-        for (name = tab, i = z; --i >= 0; )
-            *name++ = 0;
-        for (i = 0; i < ksnum; i++) {
-            name = info[i].name;
-            sig = 0;
-            while ((c = *name++))
-                sig = (sig << 1) + c;
-            first = j = sig % z;
-            for (k = 0; tab[j]; k++) {
-                j += first + 1;
-                if (j >= z)
-                    j -= z;
-                if (j == first)
-                    goto next1;
-            }
-            tab[j] = 1;
-            if (k > max_rehash)
-                max_rehash = k;
-        }
-        if (max_rehash < MIN_REHASH) {
-            if (max_rehash < best_max_rehash) {
-                best_max_rehash = max_rehash;
-                best_z = z;
-            }
-            num_found++;
-            if (num_found >= MATCHES)
-                break;
-        }
-next1:;
-    }
-
-    z = best_z;
-    printf("#ifndef KS_TABLES_H\n");
-    printf("#define KS_TABLES_H\n\n");
-    printf("static const unsigned char _XkeyTable[] = {\n");
-    if (z == 0) {
-        fprintf(stderr, "makekeys: failed to find small enough hash!\n"
-                "Try increasing KTNUM in makekeys.c\n");
-        exit(1);
-    }
-    printf("0,\n");
-    k = 1;
-    for (i = 0; i < ksnum; i++) {
-        name = info[i].name;
-        sig = 0;
-        while ((c = *name++))
-            sig = (sig << 1) + c;
-        first = j = sig % z;
-        while (offsets[j]) {
-            j += first + 1;
-            if (j >= z)
-                j -= z;
-        }
-        offsets[j] = k;
-        indexes[i] = k;
-        val = info[i].val;
-        printf("0x%.2" PRIx32 ", 0x%.2" PRIx32 ", 0x%.2" PRIx32 ", "
-               "0x%.2" PRIx32 ", 0x%.2" PRIx32 ", 0x%.2" PRIx32 ", ",
-               (sig >> 8) & 0xff, sig & 0xff, (val >> 24) & 0xff,
-               (val >> 16) & 0xff, (val >> 8) & 0xff, val & 0xff);
-        for (name = info[i].name, k += 7; (c = *name++); k++)
-            printf("'%c',", c);
-        printf((i == (ksnum - 1)) ? "0\n" : "0,\n");
-    }
-    printf("};\n");
-    printf("\n");
-    printf("#define KTABLESIZE %d\n", z);
-    printf("#define KMAXHASH %d\n", best_max_rehash + 1);
-    printf("\n");
-    printf("static const unsigned short hashString[KTABLESIZE] = {\n");
-    for (i = 0; i < z; ) {
-        printf("0x%.4x", offsets[i]);
-        i++;
-        if (i == z)
-            break;
-        printf((i & 7) ? ", " : ",\n");
-    }
-    printf("\n");
-    printf("};\n");
-
-    best_max_rehash = ksnum;
-    num_found = 0;
-    for (z = ksnum; z < KTNUM; z++) {
-        max_rehash = 0;
-        for (name = tab, i = z; --i >= 0; )
-            *name++ = 0;
-        for (i = 0; i < ksnum; i++) {
-            val = info[i].val;
-            first = j = val % z;
-            for (k = 0; tab[j]; k++) {
-                if (values[j] == val)
-                    goto skip1;
-                j += first + 1;
-                if (j >= z)
-                    j -= z;
-                if (j == first)
-                    goto next2;
-            }
-            tab[j] = 1;
-            values[j] = val;
-            if (k > max_rehash)
-                max_rehash = k;
-skip1:;
-        }
-        if (max_rehash < MIN_REHASH) {
-            if (max_rehash < best_max_rehash) {
-                best_max_rehash = max_rehash;
-                best_z = z;
-            }
-            num_found++;
-            if (num_found >= MATCHES)
-                break;
-        }
-next2:;
-    }
-
-    z = best_z;
-    if (z == 0) {
-        fprintf(stderr, "makekeys: failed to find small enough hash!\n"
-                "Try increasing KTNUM in makekeys.c\n");
-        exit(1);
-    }
-    for (i = z; --i >= 0; )
-        offsets[i] = 0;
-    for (i = 0; i < ksnum; i++) {
-        val = info[i].val;
-        first = j = val % z;
-        while (offsets[j]) {
-            if (values[j] == val)
-                goto skip2;
-            j += first + 1;
-            if (j >= z)
-                j -= z;
-        }
-        offsets[j] = indexes[i] + 2;
-        values[j] = val;
-skip2:;
-    }
-    printf("\n");
-    printf("#define VTABLESIZE %d\n", z);
-    printf("#define VMAXHASH %d\n", best_max_rehash + 1);
-    printf("\n");
-    printf("static const unsigned short hashKeysym[VTABLESIZE] = {\n");
-    for (i = 0; i < z; ) {
-        printf("0x%.4x", offsets[i]);
-        i++;
-        if (i == z)
-            break;
-        printf((i & 7) ? ", " : ",\n");
-    }
-    printf("\n");
-    printf("};\n");
-    printf("\n#endif /* KS_TABLES_H */\n");
-
-    for (i = 0; i < ksnum; i++)
-        free(info[i].name);
-
-    exit(0);
-}

src/keysym.c

@@ -47,49 +47,43 @@
  * DEALINGS IN THE SOFTWARE.
  */
 
+#include <stdlib.h>
 #include "xkbcommon/xkbcommon.h"
 #include "utils.h"
-#include "ks_tables.h"
 #include "keysym.h"
+#include "ks_tables.h"
+
+static int
+compare_by_keysym(const void *a, const void *b)
+{
+    const struct name_keysym *key = a, *entry = b;
+    return key->keysym - (int32_t)entry->keysym;
+}
+
+static int
+compare_by_name(const void *a, const void *b)
+{
+    const struct name_keysym *key = a, *entry = b;
+    return strcmp(key->name, entry->name);
+}
 
 XKB_EXPORT int
 xkb_keysym_get_name(xkb_keysym_t ks, char *buffer, size_t size)
 {
-    int i, n, h, idx;
+    const struct name_keysym search = { .name = NULL, .keysym = ks };
-    const unsigned char *entry;
+    const struct name_keysym *entry;
-    unsigned char val1, val2, val3, val4;
 
     if ((ks & ((unsigned long) ~0x1fffffff)) != 0) {
         snprintf(buffer, size, "Invalid");
         return -1;
     }
 
-    /* Try to find it in our hash table. */
+    entry = bsearch(&search, keysym_to_name,
-    if (ks <= 0x1fffffff) {
+                    sizeof(keysym_to_name) / sizeof(*keysym_to_name),
-        val1 = ks >> 24;
+                    sizeof(*keysym_to_name),
-        val2 = (ks >> 16) & 0xff;
+                    compare_by_keysym);
-        val3 = (ks >> 8) & 0xff;
+    if (entry)
-        val4 = ks & 0xff;
+        return snprintf(buffer, size, "%s", entry->name);
-        i = ks % VTABLESIZE;
-        h = i + 1;
-        n = VMAXHASH;
-
-        while ((idx = hashKeysym[i])) {
-            entry = &_XkeyTable[idx];
-
-            if ((entry[0] == val1) && (entry[1] == val2) &&
-                (entry[2] == val3) && (entry[3] == val4)) {
-                return snprintf(buffer, size, "%s", entry + 4);
-            }
-
-            if (!--n)
-                break;
-
-            i += h;
-            if (i >= VTABLESIZE)
-                i -= VTABLESIZE;
-        }
-    }
 
     if (ks >= 0x01000100 && ks <= 0x0110ffff)
         /* Unnamed Unicode codepoint. */
@@ -102,42 +96,17 @@ xkb_keysym_get_name(xkb_keysym_t ks, char *buffer, size_t size)
 XKB_EXPORT xkb_keysym_t
 xkb_keysym_from_name(const char *s)
 {
-    int i, n, h, c, idx;
+    const struct name_keysym search = { .name = s, .keysym = 0 };
-    uint32_t sig = 0;
+    const struct name_keysym *entry;
-    const char *p = s;
     char *tmp;
-    const unsigned char *entry;
-    unsigned char sig1, sig2;
     xkb_keysym_t val;
 
-    while ((c = *p++))
+    entry = bsearch(&search, name_to_keysym,
-        sig = (sig << 1) + c;
+                    sizeof(name_to_keysym) / sizeof(*name_to_keysym),
-
+                    sizeof(*name_to_keysym),
-    i = sig % KTABLESIZE;
+                    compare_by_name);
-    h = i + 1;
+    if (entry)
-    sig1 = (sig >> 8) & 0xff;
+        return entry->keysym;
-    sig2 = sig & 0xff;
-    n = KMAXHASH;
-
-    while ((idx = hashString[i])) {
-        entry = &_XkeyTable[idx];
-
-        if ((entry[0] == sig1) && (entry[1] == sig2) &&
-            streq(s, (const char *) entry + 6)) {
-            val = (entry[2] << 24) | (entry[3] << 16) |
-                  (entry[4] << 8) | entry[5];
-            if (!val)
-                val = XKB_KEY_VoidSymbol;
-            return val;
-        }
-
-        if (!--n)
-            break;
-
-        i += h;
-        if (i >= KTABLESIZE)
-            i -= KTABLESIZE;
-    }
 
     if (*s == 'U') {
         val = strtoul(&s[1], &tmp, 16);