diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 822f6e70..abc5597f 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -7,11 +7,15 @@ env: jobs: ubuntu: - name: Test ${{matrix.os}} + name: Test ${{matrix.os}} ${{matrix.build_type}} runs-on: ${{matrix.os}} strategy: matrix: os: [ubuntu-22.04, ubuntu-24.04, ubuntu-26.04] + # Release defines NDEBUG, which makes every ASSERT() a no-op. Testing + # only Debug cannot catch a defect that an assertion was masking, and + # Release is what actually ships. + build_type: [Debug, Release] steps: - uses: actions/checkout@v7 @@ -26,17 +30,45 @@ jobs: - name: Configure CMake shell: bash working-directory: ${{github.workspace}}/build - run: cmake $GITHUB_WORKSPACE -DCMAKE_BUILD_TYPE=$BUILD_TYPE + run: cmake $GITHUB_WORKSPACE -DCMAKE_BUILD_TYPE=${{matrix.build_type}} - name: Build working-directory: ${{github.workspace}}/build shell: bash - run: cmake --build . --config $BUILD_TYPE + run: cmake --build . --config ${{matrix.build_type}} - name: Test working-directory: ${{github.workspace}}/build shell: bash - run: ctest -C $BUILD_TYPE + run: ctest -C ${{matrix.build_type}} --output-on-failure + + sanitizers: + name: Test sanitizers + runs-on: ubuntu-24.04 + + steps: + - uses: actions/checkout@v7 + + - name: Install dependencies + shell: bash + run: sudo apt update && sudo apt install libcunit1-dev --yes + + - name: Build and run the test suite under ASan, UBSan and LeakSanitizer + shell: bash + run: | + gcc -std=c11 -g -O1 -fsanitize=address,undefined \ + -fno-omit-frame-pointer -fno-sanitize-recover=all \ + -Iinclude -Isrc src/*.c unit_tests.c -o unit_tests_asan -lcunit + ASAN_OPTIONS=detect_leaks=1 ./unit_tests_asan + + - name: Build the library with warnings as errors + shell: bash + run: | + for f in src/*.c; do \ + gcc -c -std=c11 -O2 -Werror -Wall -Wextra -Wshadow -Wcast-qual \ + -Wstrict-prototypes -Wmissing-prototypes -Wsign-compare \ + -Wpointer-arith -Iinclude -Isrc "$f" -o /dev/null; \ + done debian: name: Test debian ${{matrix.os}} diff --git a/README.md b/README.md index 558f45f2..381785d0 100644 --- a/README.md +++ b/README.md @@ -13,13 +13,15 @@ libdict is a C library that provides the following data structures with efficien * [weight-balanced tree](https://en.wikipedia.org/wiki/Weight-balanced_tree) * [path-reduction tree](https://cs.uwaterloo.ca/research/tr/1982/CS-82-07.pdf) * [treap](http://en.wikipedia.org/wiki/Treap) -* [hashtable using separate chaining](http://en.wikipedia.org/wiki/Hashtable#Separate_chaining) -* [hashtable using open addressing with linear probing](http://en.wikipedia.org/wiki/Hashtable#Open_addressing) +* [hashtable using separate chaining](http://en.wikipedia.org/wiki/Hash_table#Separate_chaining) +* [hashtable using open addressing with linear probing](http://en.wikipedia.org/wiki/Hash_table#Open_addressing) * [skip list](https://en.wikipedia.org/wiki/Skip_list) -All data structures in this library support insert, search, and remove, and have bidirectional iterators. The sorted data structures (everything but hash tables) support near-search operations: searching for the key greater or equal to, strictly greater than, lesser or equal to, or strictly less than, a given key. The tree data structures also support the selecting the nth element; this takes linear time, except in path-reduction and weight-balanced trees, where it only takes logarithmic time. +All data structures in this library support insert, search, and remove, and have bidirectional iterators. The sorted data structures (everything but hash tables) support near-search operations: searching for the key greater or equal to, strictly greater than, lesser or equal to, or strictly less than, a given key. The tree data structures also support selecting the nth element; this takes linear time, except in path-reduction and weight-balanced trees, where it only takes logarithmic time. -The API and code are written with efficiency as a primary concern. For example, an insert call returns a boolean indicating whether or not the key was already present in the dictionary (i.e. whether there was an insertion or a collision), and a pointer to the location of the associated data. Thus, an insert-or-update operation can be supported with a single traversal of the data structure. In addition, almost all recursive algorithms have been rewritten to use iteration instead. +Iterator `remove` and `compare` are supported on all containers, including the hash tables. + +The API and code are written with efficiency as a primary concern. For example, an insert call returns a boolean (`dict_insert_result.inserted`) indicating whether or not the key was inserted, i.e. `true` when the key was not already present and `false` when an existing entry was found, and a pointer to the location of the associated data. Thus, an insert-or-update operation can be supported with a single traversal of the data structure. In addition, almost all recursive algorithms have been rewritten to use iteration instead. Documentation is generated by Doxygen on every commit to master and is available [here](https://rtbrick.github.io/libdict/html). diff --git a/TODO b/TODO index 309e6e80..a54fc886 100644 --- a/TODO +++ b/TODO @@ -1,7 +1,7 @@ [ ] Use restrict keyword wherever appropriate. [X] Fix skiplist prev pointers. -[ ] Implement incomplete functionality, e.g. iterator remove & compare. -[X] Reformat to 80 columns. +[X] Implement incomplete functionality, e.g. iterator remove & compare. +[ ] Reformat to 80 columns. [ ] Optimize double-rotations. [X] Fix bugs in weight balanced tree. [ ] Optimize skiplist. diff --git a/anagram.c b/anagram.c index aec68ff1..c14c79b2 100644 --- a/anagram.c +++ b/anagram.c @@ -15,6 +15,15 @@ struct WordList { WordList *next; }; +/* The tree owns the keys; the WordList data are freed while iterating, so + * only the key is released here. */ +static void +key_free(void *key, void *datum) +{ + (void)datum; + free(key); +} + int main(int argc, char *argv[]) { @@ -29,37 +38,59 @@ main(int argc, char *argv[]) exit(1); } + dict_malloc_func = xmalloc; + rb_tree *tree = rb_tree_new(dict_str_cmp); char buf[512]; while (fgets(buf, sizeof(buf), fp)) { - if (isupper(buf[0])) /* Disregard proper nouns. */ + if (isupper((unsigned char) buf[0])) /* Disregard proper nouns. */ continue; strtok(buf, "\r\n"); int freq[256] = { 0 }; - memset(freq, 0, sizeof(freq)); - - ASSERT(buf[0] != '\0'); for (char *p = buf; *p; p++) - freq[tolower(*p)]++; + freq[tolower((unsigned char) *p)]++; + + /* The signature encodes each letter count as a single digit, so a + * word with ten or more of the same letter cannot be represented + * unambiguously; skip it rather than mis-group it. */ + int representable = 1; + for (int i = 1; i < 256; i++) { + if (freq[i] > 9) { + representable = 0; + break; + } + } + if (!representable) { + fprintf(stderr, "Skipping '%s': letter repeated 10 or more times.\n", + buf); + continue; + } char name[1024]; char *p = name; for (int i=1; i<256; i++) { if (freq[i]) { - ASSERT(freq[i] < 10); - *p++ = (char) i; *p++ = '0' + (char) freq[i]; } } *p = 0; + char *key = xstrdup(name); + dict_insert_result result = rb_tree_insert(tree, key); + if (!result.datum_ptr) { + free(key); + fprintf(stderr, "Insertion failed\n"); + exit(1); + } + if (!result.inserted) + free(key); /* The tree kept the pre-existing key. */ WordList* word = xmalloc(sizeof(*word)); word->word = xstrdup(buf); - WordList** wordp = (WordList**) rb_tree_insert(tree, xstrdup(name)).datum_ptr; + WordList** wordp = (WordList**) result.datum_ptr; word->next = *wordp; *wordp = word; } @@ -87,9 +118,9 @@ main(int argc, char *argv[]) free(word); word = next; } - } while (rb_itor_next(itor)); + } rb_itor_free(itor); - rb_tree_free(tree, NULL); + rb_tree_free(tree, key_free); fclose(fp); return 0; diff --git a/benchmark.c b/benchmark.c index 0dc4a09a..aa0585db 100644 --- a/benchmark.c +++ b/benchmark.c @@ -24,7 +24,7 @@ static const char appname[] = "benchmark"; char *xstrdup(const char *str); -#ifdef __GNUC__ +#if defined(__GNUC__) || defined(__clang__) # define NORETURN __attribute__((__noreturn__)) #else # define NORETURN @@ -36,8 +36,6 @@ void *xmalloc(size_t size); static size_t hash_count = 0, comp_count = 0; unsigned str_hash(const void *p); int my_strcmp(const void *k1, const void *k2); -unsigned ptr_hash(const void *p); -int my_ptrcmp(const void *k1, const void *k2); void key_str_free(void *key, void *datum); void timer_start(struct rusage* start); @@ -54,8 +52,6 @@ static size_t malloced = 0; int main(int argc, char **argv) { - bool shuffle_keys = true; - if (argc != 3) { fprintf(stderr, "usage: %s [type] [input]\n", appname); fprintf(stderr, "type: specifies the dictionary type:\n"); @@ -92,24 +88,27 @@ main(int argc, char **argv) if (fp == NULL) quit("cant open file '%s': %s", argv[2], strerror(errno)); - size_t nwords = 0; + size_t nlines = 0; char buf[512]; while (fgets(buf, sizeof(buf), fp)) - ++nwords; + ++nlines; - if (!nwords) + if (!nlines) quit("nothing read from file"); - char **words = xmalloc(sizeof(*words) * nwords); + char **words = xmalloc(sizeof(*words) * nlines); rewind(fp); - size_t words_read = 0; - while (words_read < nwords && fgets(buf, sizeof(buf), fp)) { - strtok(buf, "\n"); - words[words_read++] = xstrdup(buf); + size_t nwords = 0; + while (nwords < nlines && fgets(buf, sizeof(buf), fp)) { + char *word = strtok(buf, "\n"); + if (!word || !*word) /* Skip blank lines: empty keys are useless + * here, and would divide by zero below. */ + continue; + words[nwords++] = xstrdup(word); } fclose(fp); - if (words_read < nwords) - quit("Only read %zu/%zu words!", words_read, nwords); + if (!nwords) + quit("no non-empty keys read from file"); printf("Loaded %zu keys from %s.\n", nwords, argv[2]); malloced = malloced_save; @@ -122,7 +121,7 @@ main(int argc, char **argv) for (unsigned i = 0; i < nwords; i++) { dict_insert_result result = dict_insert(dct, words[i]); if (!result.inserted) - quit("insert #%d failed for '%s'", i, words[i]); + quit("insert #%u failed for '%s'", i, words[i]); ASSERT(result.datum_ptr != NULL); ASSERT(*result.datum_ptr == NULL); *result.datum_ptr = words[i]; @@ -149,13 +148,18 @@ main(int argc, char **argv) tree->rotation_count = 0; } else if (type == 'S') { size_t counts[16] = { 0 }; - size_t num_counts = skiplist_link_count_histogram(dict_private(dct), counts, sizeof(counts) / sizeof(counts[0])); + const size_t ncounts = sizeof(counts) / sizeof(counts[0]); + /* Returns the maximum link count seen; only indices < ncounts are + * written, so nodes with that many links or more are not counted. */ + const size_t max_links = + skiplist_link_count_histogram(dict_private(dct), counts, ncounts); + const size_t limit = (max_links < ncounts) ? max_links + 1 : ncounts; size_t count_sum = 0; - for (size_t i = 0; i <= num_counts; ++i) { + for (size_t i = 0; i < limit; ++i) { printf("skiplist %zu-node(s): %zu\n", i, counts[i]); count_sum += counts[i]; } - ASSERT(count_sum == nwords); + ASSERT(max_links < ncounts ? count_sum == nwords : count_sum <= nwords); } ASSERT(dict_verify(dct)); @@ -163,7 +167,7 @@ main(int argc, char **argv) size_t n = dict_count(dct); if (n != nwords) - quit("bad count (%u - should be %u)!", n, nwords); + quit("bad count (%zu - should be %zu)!", n, nwords); dict_itor *itor = dict_itor_new(dct); @@ -180,7 +184,7 @@ main(int argc, char **argv) container_name, (end.ru_utime.tv_sec * 1000000 + end.ru_utime.tv_usec) * 1e-6); if (n != nwords) - warn("Fwd iteration returned %u items - should be %u", n, nwords); + warn("Fwd iteration returned %zu items - should be %zu", n, nwords); ASSERT(dict_verify(dct)); comp_count = hash_count = 0; /* Ignore comparisons/hashes incurred by dict_verify() */ @@ -198,11 +202,11 @@ main(int argc, char **argv) container_name, (end.ru_utime.tv_sec * 1000000 + end.ru_utime.tv_usec) * 1e-6); if (n != nwords) - warn("Rev iteration returned %u items - should be %u", n, nwords); + warn("Rev iteration returned %zu items - should be %zu", n, nwords); dict_itor_free(itor); - if (shuffle_keys) shuffle(words, nwords); + shuffle(words, nwords); ASSERT(dict_verify(dct)); comp_count = hash_count = 0; /* Ignore comparisons/hashes incurred by dict_verify() */ @@ -211,7 +215,7 @@ main(int argc, char **argv) for (unsigned i = 0; i < nwords; i++) { void **p = dict_search(dct, words[i]); if (!p) - quit("lookup failed for '%s'", buf); + quit("lookup failed for '%s'", words[i]); if (*p != words[i]) quit("bad data for '%s', got '%s' instead", words[i], *(char **)p); } @@ -256,15 +260,19 @@ main(int argc, char **argv) ASSERT(dict_verify(dct)); comp_count = hash_count = 0; /* Ignore comparisons/hashes incurred by dict_verify() */ - if (shuffle_keys) shuffle(words, nwords); + shuffle(words, nwords); timer_start(&start); for (unsigned i = 0; i < nwords; i++) { dict_remove_result result = dict_remove(dct, words[i]); if (!result.removed) - quit("removing #%d '%s' failed!\n", i, words[i]); + quit("removing #%u '%s' failed!\n", i, words[i]); ASSERT(result.key == words[i]); ASSERT(result.datum == words[i]); + /* The dict no longer owns the key, so release it here; otherwise + * the dict_free() below has nothing left to free. */ + free(result.key); + words[i] = NULL; } timer_end(&start, &end, &total); printf(" %s remove: %6.03fs %9zu cmp (%.2f/remove)", @@ -287,7 +295,7 @@ main(int argc, char **argv) comp_count = hash_count = 0; /* Ignore comparisons/hashes incurred by dict_verify() */ if ((n = dict_count(dct)) != 0) - quit("error - count not zero (%u)!", n); + quit("error - count not zero (%zu)!", n); dict_free(dct, key_str_free); @@ -303,7 +311,10 @@ main(int argc, char **argv) printf(" total rotations: %zu\n", total_rotations); } - FREE(words); + /* words was allocated with xmalloc()/malloc(), not through the library's + * allocator, so release it with plain free(). Note that the 'malloced' + * accounting is not decremented. */ + free(words); exit(EXIT_SUCCESS); } @@ -352,8 +363,10 @@ create_dictionary(char type, const char **container_name) return hashtable2_dict_new(cmp_func, hash_func, HASHTABLE_SIZE); default: - quit("type must be one of h, p, r, t, s, w or H"); + quit("type must be one of h, p, r, t, s, w, S, H or 2"); } + /* Not reached: quit() does not return. */ + abort(); } char * @@ -413,20 +426,6 @@ my_strcmp(const void *k1, const void *k2) return strcmp(k1, k2); } -unsigned -ptr_hash(const void *p) -{ - ++hash_count; - return (unsigned) ((2166136261U ^ (uintptr_t)p) * 16777619U); -} - -int -my_ptrcmp(const void *k1, const void *k2) -{ - ++comp_count; - return (k1 < k2) ? -1 : (k1 > k2); -} - void key_str_free(void *key, void *datum) { @@ -450,7 +449,7 @@ timer_end(const struct rusage *start, struct rusage *end, end->ru_utime.tv_usec -= start->ru_utime.tv_usec; end->ru_utime.tv_sec -= start->ru_utime.tv_sec; total->tv_sec += end->ru_utime.tv_sec; - if ((total->tv_usec += end->ru_utime.tv_usec) > 1000000) { + if ((total->tv_usec += end->ru_utime.tv_usec) >= 1000000) { total->tv_usec -= 1000000; total->tv_sec++; } } diff --git a/demo.c b/demo.c index b2cca6d0..e8dcd670 100644 --- a/demo.c +++ b/demo.c @@ -24,7 +24,6 @@ char *xstrdup(const char *str); #endif void quit(const char *, ...) NORETURN; void *xmalloc(size_t size); -void *xrealloc(void *ptr, size_t size); void *xdup(const void *ptr, size_t size); static void @@ -96,10 +95,10 @@ main(int argc, char **argv) char *p, *ptr, *ptr2; if ((p = strchr(buf, '\n')) != NULL) *p = 0; - for (p = buf; *p && isspace(*p); p++) + for (p = buf; *p && isspace((unsigned char) *p); p++) /* void */; if (buf != p) { - strcpy(buf, p); + memmove(buf, p, strlen(p) + 1); } ptr2 = (ptr = strtok(buf, " ") ? strtok(NULL, " ") : NULL) ? strtok(NULL, " ") : NULL; @@ -110,17 +109,19 @@ main(int argc, char **argv) printf("usage: insert \n"); continue; } - dict_insert_result result = dict_insert(dct, xstrdup(ptr)); + char *key = xstrdup(ptr); + dict_insert_result result = dict_insert(dct, key); if (result.inserted) { *result.datum_ptr = xstrdup(ptr2); printf("inserted '%s': '%s'\n", ptr, (char *)*result.datum_ptr); } else { + free(key); /* The dict kept the pre-existing key. */ printf("'%s' already in dict: '%s'\n", ptr, (char *)*result.datum_ptr); } } else if (strcmp(buf, "search") == 0) { - if (ptr2) { + if (!ptr || ptr2) { printf("usage: search \n"); continue; } @@ -130,12 +131,12 @@ main(int argc, char **argv) else printf("'%s' not found!\n", ptr); } else if (strcmp(buf, "searchle") == 0) { - if (ptr2) { + if (!ptr || ptr2) { printf("usage: searchle \n"); continue; } if (!dict_is_sorted(dct)) { - printf("dict does not support that operation!"); + printf("dict does not support that operation!\n"); continue; } void** search = dict_search_le(dct, ptr); @@ -144,12 +145,12 @@ main(int argc, char **argv) else printf("le '%s': no result.\n", ptr); } else if (strcmp(buf, "searchlt") == 0) { - if (ptr2) { + if (!ptr || ptr2) { printf("usage: searchlt \n"); continue; } if (!dict_is_sorted(dct)) { - printf("dict does not support that operation!"); + printf("dict does not support that operation!\n"); continue; } void** search = dict_search_lt(dct, ptr); @@ -158,12 +159,12 @@ main(int argc, char **argv) else printf("lt '%s': no result.\n", ptr); } else if (strcmp(buf, "searchge") == 0) { - if (ptr2) { + if (!ptr || ptr2) { printf("usage: searchge \n"); continue; } if (!dict_is_sorted(dct)) { - printf("dict does not support that operation!"); + printf("dict does not support that operation!\n"); continue; } void** search = dict_search_ge(dct, ptr); @@ -172,12 +173,12 @@ main(int argc, char **argv) else printf("ge '%s': no result.\n", ptr); } else if (strcmp(buf, "searchgt") == 0) { - if (ptr2) { + if (!ptr || ptr2) { printf("usage: searchgt \n"); continue; } if (!dict_is_sorted(dct)) { - printf("dict does not support that operation!"); + printf("dict does not support that operation!\n"); continue; } void** search = dict_search_gt(dct, ptr); @@ -206,7 +207,7 @@ main(int argc, char **argv) dict_itor_first(itor); for (; dict_itor_valid(itor); dict_itor_next(itor)) printf("'%s': '%s'\n", - (char *)dict_itor_key(itor), + (const char *)dict_itor_key(itor), (char *)*dict_itor_datum(itor)); dict_itor_free(itor); } else if (strcmp(buf, "reverse") == 0) { @@ -218,7 +219,7 @@ main(int argc, char **argv) dict_itor_last(itor); for (; dict_itor_valid(itor); dict_itor_prev(itor)) printf("'%s': '%s'\n", - (char *)dict_itor_key(itor), + (const char *)dict_itor_key(itor), (char *)*dict_itor_datum(itor)); dict_itor_free(itor); } else if (strcmp(buf, "clear") == 0) { diff --git a/include/dict.h b/include/dict.h index c4f824ea..e83c977c 100644 --- a/include/dict.h +++ b/include/dict.h @@ -28,20 +28,34 @@ #ifndef LIBDICT_DICT_H__ #define LIBDICT_DICT_H__ -#if defined(__cplusplus) || defined(c_plusplus) -# define BEGIN_DECL extern "C" { -# define END_DECL } -#else -# define BEGIN_DECL -# define END_DECL -#endif - -BEGIN_DECL - +/* Include system headers before opening the extern "C" block: in C++ these + * can pull in namespace-scoped declarations, which are ill-formed with C + * language linkage. */ #include #include #include +#ifndef LIBDICT_BEGIN_DECL +# if defined(__cplusplus) || defined(c_plusplus) +# define LIBDICT_BEGIN_DECL extern "C" { +# define LIBDICT_END_DECL } +# else +# define LIBDICT_BEGIN_DECL +# define LIBDICT_END_DECL +# endif +#endif + +/* Deprecated unprefixed aliases, kept for source compatibility. Defined only + * if nothing else has claimed the names. */ +#ifndef BEGIN_DECL +# define BEGIN_DECL LIBDICT_BEGIN_DECL +#endif +#ifndef END_DECL +# define END_DECL LIBDICT_END_DECL +#endif + +LIBDICT_BEGIN_DECL + #define DICT_VERSION_MAJOR 0 #define DICT_VERSION_MINOR 3 #define DICT_VERSION_PATCH 0 @@ -49,8 +63,10 @@ extern const char* const kDictVersionString; /* A pointer to a function that compares two keys. It needs to return a * negative value if k1k2, and zero if the keys are - * equal. The comparison should be reflexive (k1>k2 implies k1k2 and k2>k3 implies k1>k3). */ + * equal. The comparison must be a total order, i.e. reflexive (cmp(k,k)==0), + * antisymmetric (k1>k2 implies k2k2 and k2>k3 implies + * k1>k3). Keys must compare identically for as long as they are in the + * dictionary. */ typedef int (*dict_compare_func)(const void*, const void*); /* A pointer to a function that is called when a key-value pair gets removed * from a dictionary. */ @@ -59,7 +75,9 @@ typedef void (*dict_delete_func)(void*, void*); typedef bool (*dict_visit_func)(const void*, void*, void*); /* A pointer to a function that returns the hash value of a key. */ typedef unsigned (*dict_hash_func)(const void*); -/* A pointer to a function that returns the priority of a key. */ +/* A pointer to a function that returns the priority of a key, used by tr_tree + * (a treap). The treap is a max-heap on priority, so a LARGER value places the + * key closer to the root. Pass NULL to have priorities chosen at random. */ typedef unsigned (*dict_prio_func)(const void*); /* A pointer to a function that libdict will use to allocate memory. */ @@ -67,7 +85,7 @@ extern void* (*dict_malloc_func)(size_t); /* A pointer to a function that libdict will use to deallocate memory. */ extern void (*dict_free_func)(void*); -/* Forward declarations for transparent type dict_itor. */ +/* Forward declaration of dict_itor, whose definition appears below. */ typedef struct dict_itor dict_itor; typedef struct { @@ -121,16 +139,18 @@ typedef bool (*dict_nextn_func)(void* itor, size_t count); typedef bool (*dict_prevn_func)(void* itor, size_t count); typedef bool (*dict_first_func)(void* itor); typedef bool (*dict_last_func)(void* itor); -typedef void* (*dict_key_func)(void* itor); +/* Returns the key the iterator is positioned on, or NULL if it is not + * positioned. The key belongs to the dictionary and must not be modified. */ +typedef const void* (*dict_key_func)(const void* itor); typedef void** (*dict_datum_func)(void* itor); typedef bool (*dict_isearch_func)(void* itor, const void* key); typedef bool (*dict_iremove_func)(void* itor); -typedef int (*dict_icompare_func)(void* itor1, void* itor2); +typedef int (*dict_icompare_func)(const void* itor1, const void* itor2); typedef struct { dict_ifree_func ifree; dict_valid_func valid; - dict_invalidate_func invalid; + dict_invalidate_func invalidate; dict_next_func next; dict_prev_func prev; dict_nextn_func nextn; @@ -164,6 +184,11 @@ typedef struct { #define dict_remove(dct,key) ((dct)->_vtable->remove((dct)->_object, (key))) #define dict_clear(dct,func) ((dct)->_vtable->clear((dct)->_object, (func))) #define dict_traverse(dct,func,ud) ((dct)->_vtable->traverse((dct)->_object, (func), (ud))) +/* Retrieve the |n|th element in key order (0-based), storing its key in |key| + * and its datum in |d|, and evaluating to true. Evaluates to false, storing + * NULL through any non-NULL out-parameter, if |n| is out of range or the + * container does not support selection. Either out-parameter may be NULL. + * Only meaningful for sorted containers. */ #define dict_select(dct,n,key,d) ((dct)->_vtable->select && (dct)->_vtable->select((dct)->_object, (n), (key), (d))) #define dict_count(dct) ((dct)->_vtable->count((dct)->_object)) #define dict_verify(dct) ((dct)->_vtable->verify((dct)->_object)) @@ -171,6 +196,25 @@ typedef struct { size_t dict_free(dict* dct, dict_delete_func delete_func); +/* ITERATOR INVALIDATION + * + * Removing an element invalidates iterators as follows. + * + * - For the hash tables, ANY structural modification (an insert that grows + * the table, any removal, or a clear) invalidates every existing iterator: + * dict_itor_valid() then returns false, dict_itor_key()/_datum() return + * NULL, and dict_itor_next()/_prev() return false. Reposition with + * dict_itor_first()/_last()/_search() to use the iterator again. + * + * - For the trees and the skip list, removing an element invalidates only + * iterators positioned on a removed node. Note that for every tree except + * tr_tree, removing a key with two children frees the node that held its + * SUCCESSOR (the key and datum are moved into the surviving node), so an + * iterator parked on the successor is invalidated too even though its own + * key was not removed. + * + * Inserting into a tree or skip list never invalidates an iterator. */ + struct dict_itor { void* _itor; const itor_vtable* _vtable; @@ -178,7 +222,7 @@ struct dict_itor { #define dict_itor_private(i) ((i)->_itor) #define dict_itor_valid(i) ((i)->_vtable->valid((i)->_itor)) -#define dict_itor_invalidate(i) ((i)->_vtable->invalid((i)->_itor)) +#define dict_itor_invalidate(i) ((i)->_vtable->invalidate((i)->_itor)) #define dict_itor_next(i) ((i)->_vtable->next((i)->_itor)) #define dict_itor_prev(i) ((i)->_vtable->prev((i)->_itor)) #define dict_itor_nextn(i,n) ((i)->_vtable->nextn((i)->_itor, (n))) @@ -192,7 +236,12 @@ struct dict_itor { #define dict_itor_search_gt(i,k) ((i)->_vtable->search_gt && (i)->_vtable->search_gt((i)->_itor, (k))) #define dict_itor_key(i) ((i)->_vtable->key((i)->_itor)) #define dict_itor_datum(i) ((i)->_vtable->datum((i)->_itor)) -#define dict_itor_compare(i1,i2) ((i1)->_vtable->compare((i1)->_itor, (i2)->_itor)) +/* Compare the positions of two iterators over the same dictionary. Returns a + * negative value, zero or a positive value as |i1| precedes, is at, or follows + * |i2|. For unsorted containers the order is the container's current traversal + * order, which is only meaningful until the next structural modification. + * Evaluates to 0 if the container supplies no comparison. */ +#define dict_itor_compare(i1,i2) ((i1)->_vtable->compare ? (i1)->_vtable->compare((i1)->_itor, (i2)->_itor) : 0) #define dict_itor_remove(i) ((i)->_vtable->remove && (i)->_vtable->remove((i)->_itor)) void dict_itor_free(dict_itor* itor); diff --git a/include/hashtable.h b/include/hashtable.h index 3bc1b06f..43c232e6 100644 --- a/include/hashtable.h +++ b/include/hashtable.h @@ -69,6 +69,7 @@ bool hashtable_itor_search(hashtable_itor* itor, const void* key); const void* hashtable_itor_key(const hashtable_itor* itor); void** hashtable_itor_datum(hashtable_itor* itor); bool hashtable_itor_remove(hashtable_itor* itor); +int hashtable_itor_compare(const hashtable_itor* i1, const hashtable_itor* i2); END_DECL diff --git a/include/hashtable2.h b/include/hashtable2.h index bd9bd66d..c85b8fa1 100644 --- a/include/hashtable2.h +++ b/include/hashtable2.h @@ -69,6 +69,7 @@ bool hashtable2_itor_search(hashtable2_itor* itor, const void* key); const void* hashtable2_itor_key(const hashtable2_itor* itor); void** hashtable2_itor_datum(hashtable2_itor* itor); bool hashtable2_itor_remove(hashtable2_itor* itor); +int hashtable2_itor_compare(const hashtable2_itor* i1, const hashtable2_itor* i2); END_DECL diff --git a/include/hb_tree.h b/include/hb_tree.h index cc509ae5..c9a106ee 100644 --- a/include/hb_tree.h +++ b/include/hb_tree.h @@ -60,7 +60,7 @@ typedef struct hb_itor hb_itor; hb_itor* hb_itor_new(hb_tree* tree); dict_itor* hb_dict_itor_new(hb_tree* tree); -void hb_itor_free(hb_itor* tree); +void hb_itor_free(hb_itor* itor); bool hb_itor_valid(const hb_itor* itor); void hb_itor_invalidate(hb_itor* itor); diff --git a/include/pr_tree.h b/include/pr_tree.h index cef8f995..91f7f9cf 100644 --- a/include/pr_tree.h +++ b/include/pr_tree.h @@ -60,7 +60,7 @@ typedef struct pr_itor pr_itor; pr_itor* pr_itor_new(pr_tree* tree); dict_itor* pr_dict_itor_new(pr_tree* tree); -void pr_itor_free(pr_itor* tree); +void pr_itor_free(pr_itor* itor); bool pr_itor_valid(const pr_itor* itor); void pr_itor_invalidate(pr_itor* itor); diff --git a/include/rb_tree.h b/include/rb_tree.h index 1d738add..629268bb 100644 --- a/include/rb_tree.h +++ b/include/rb_tree.h @@ -60,7 +60,7 @@ typedef struct rb_itor rb_itor; rb_itor* rb_itor_new(rb_tree* tree); dict_itor* rb_dict_itor_new(rb_tree* tree); -void rb_itor_free(rb_itor* tree); +void rb_itor_free(rb_itor* itor); bool rb_itor_valid(const rb_itor* itor); void rb_itor_invalidate(rb_itor* itor); diff --git a/include/skiplist.h b/include/skiplist.h index 850828ec..eb0b96b3 100644 --- a/include/skiplist.h +++ b/include/skiplist.h @@ -35,6 +35,9 @@ BEGIN_DECL typedef struct skiplist skiplist; skiplist* skiplist_new(dict_compare_func cmp_func, unsigned max_link); +/* |max_link| is the number of levels in the list. It is clamped into the + * usable range [2, 32]: a node receives at most max_link - 1 links, so fewer + * than 2 levels could not link anything. */ dict* skiplist_dict_new(dict_compare_func cmp_func, unsigned max_link); size_t skiplist_free(skiplist* list, dict_delete_func delete_func); @@ -51,10 +54,15 @@ dict_remove_result size_t skiplist_clear(skiplist* list, dict_delete_func delete_func); size_t skiplist_traverse(skiplist* list, dict_visit_func visit, void* user_data); size_t skiplist_count(const skiplist* list); +/* Store the key and datum of the |n|th element in key order (0-based) and + * return true, or store NULL through each non-NULL out-parameter and return + * false if |n| is out of range. Either out-parameter may be NULL. Runs in + * O(n), as no span counts are maintained. */ +bool skiplist_select(skiplist* list, size_t n, const void** key, void** datum); bool skiplist_verify(const skiplist* list); /* Compute the histogram of link counts of the skiplist. - * For 0 < x < |ncounts|, |counts|[x] will be set to the number of nodes with x + * For 0 <= x < |ncounts|, |counts|[x] will be set to the number of nodes with x * links, and the maximal link count will be returned. If the return value is * greater than or equal to |ncounts|, not all link counts could be stored in * |counts| (i.e. the array was not large enough). */ @@ -65,7 +73,7 @@ typedef struct skiplist_itor skiplist_itor; skiplist_itor* skiplist_itor_new(skiplist* list); dict_itor* skiplist_dict_itor_new(skiplist* list); -void skiplist_itor_free(skiplist_itor* ); +void skiplist_itor_free(skiplist_itor* itor); bool skiplist_itor_valid(const skiplist_itor* itor); void skiplist_itor_invalidate(skiplist_itor* itor); diff --git a/include/sp_tree.h b/include/sp_tree.h index 760e07b7..3d9460a1 100644 --- a/include/sp_tree.h +++ b/include/sp_tree.h @@ -60,7 +60,7 @@ typedef struct sp_itor sp_itor; sp_itor* sp_itor_new(sp_tree* tree); dict_itor* sp_dict_itor_new(sp_tree* tree); -void sp_itor_free(sp_itor* tree); +void sp_itor_free(sp_itor* itor); bool sp_itor_valid(const sp_itor* itor); void sp_itor_invalidate(sp_itor* itor); diff --git a/include/tr_tree.h b/include/tr_tree.h index 53079119..9585ead7 100644 --- a/include/tr_tree.h +++ b/include/tr_tree.h @@ -60,7 +60,7 @@ typedef struct tr_itor tr_itor; tr_itor* tr_itor_new(tr_tree* tree); dict_itor* tr_dict_itor_new(tr_tree* tree); -void tr_itor_free(tr_itor* tree); +void tr_itor_free(tr_itor* itor); bool tr_itor_valid(const tr_itor* itor); void tr_itor_invalidate(tr_itor* itor); diff --git a/include/wb_tree.h b/include/wb_tree.h index 3c0047d5..d5c07e1e 100644 --- a/include/wb_tree.h +++ b/include/wb_tree.h @@ -61,7 +61,7 @@ typedef struct wb_itor wb_itor; wb_itor* wb_itor_new(wb_tree* tree); dict_itor* wb_dict_itor_new(wb_tree* tree); -void wb_itor_free(wb_itor* tree); +void wb_itor_free(wb_itor* itor); bool wb_itor_valid(const wb_itor* itor); void wb_itor_invalidate(wb_itor* itor); diff --git a/src/dict.c b/src/dict.c index 26a358f2..9da52f67 100644 --- a/src/dict.c +++ b/src/dict.c @@ -72,17 +72,25 @@ dict_ulong_cmp(const void* k1, const void* k2) int dict_ptr_cmp(const void* k1, const void* k2) { - return (k1 > k2) - (k1 < k2); + /* Relational operators on pointers into different objects are undefined + * (C11 6.5.8p5); comparing the converted integers is well defined. */ + const uintptr_t a = (uintptr_t) k1; + const uintptr_t b = (uintptr_t) k2; + return (a > b) - (a < b); } int dict_str_cmp(const void* k1, const void* k2) { - const char* a = k1; - const char* b = k2; + /* Bytes must be compared as unsigned char, exactly as strcmp() is + * specified to: comparing as plain char makes 0xFF sort below 'a' on + * signed-char targets and above it on unsigned-char ones, so identical + * inputs would order differently on x86 and ARM. */ + const unsigned char* a = k1; + const unsigned char* b = k2; for (;;) { - char p = *a++, q = *b++; + unsigned char p = *a++, q = *b++; if (!p || p != q) return (p > q) - (p < q); } @@ -91,7 +99,7 @@ dict_str_cmp(const void* k1, const void* k2) unsigned dict_str_hash(const void* k) { - /* FNV 1-a string hash. */ + /* FNV-1a string hash. */ unsigned hash = 2166136261U; for (const uint8_t* ptr = k; *ptr;) { hash = (hash ^ *ptr++) * 16777619U; diff --git a/src/dict_private.h b/src/dict_private.h index c421a95c..953b7d43 100644 --- a/src/dict_private.h +++ b/src/dict_private.h @@ -101,7 +101,40 @@ # define GCC_CONST #endif +/* random() is POSIX rather than ISO C, and is not declared by under + * a strict ISO compilation, so declare it explicitly. Toolchains that do not + * provide it fall back to combining rand() calls, since RAND_MAX is only + * guaranteed to be at least 32767. */ +#if defined(_MSC_VER) || defined(_WIN32) +static inline unsigned dict_rand(void) { + return ((unsigned) rand() << 30) ^ ((unsigned) rand() << 15) ^ (unsigned) rand(); +} +#else extern long random(void); -static inline unsigned dict_rand() { return (unsigned) random(); } +static inline unsigned dict_rand(void) { return (unsigned) random(); } +#endif + +/* Number of low-order bits of dict_rand() that are usable: random() is + * specified to return a value in [0, 2^31), so the top bit of an unsigned is + * always clear. Callers needing a full-width value must combine two calls. */ +#define DICT_RAND_BITS 31 + +/* Count trailing zero bits of |n|, which must be nonzero. Unlike + * __builtin_ctz() this is available on every toolchain. */ +static inline unsigned +dict_ctz(unsigned n) +{ + ASSERT(n != 0); +#if defined(__GNUC__) || defined(__clang__) + return (unsigned) __builtin_ctz(n); +#else + unsigned count = 0; + while (!(n & 1u)) { + n >>= 1; + ++count; + } + return count; +#endif +} #endif /* !LIBDICT_DICT_PRIVATE_H__ */ diff --git a/src/hashtable.c b/src/hashtable.c index 55974ee9..42c9f88e 100644 --- a/src/hashtable.c +++ b/src/hashtable.c @@ -31,6 +31,7 @@ #include "hashtable.h" +#include /* For SIZE_MAX */ #include /* For memset() */ #include "dict_private.h" #include "hashtable_common.h" @@ -60,51 +61,86 @@ struct hashtable { dict_hash_func hash_func; size_t count; unsigned size; + /* Bumped by every structural modification (node added or removed, table + * resized). Iterators record it so they can detect that the nodes and + * slots they cached are no longer valid, instead of walking freed memory + * or resuming at a stale slot in a rebucketed table. */ + unsigned gen; }; struct hashtable_itor { hashtable* table; hash_node* node; unsigned slot; + unsigned gen; }; +/* Vtable thunks. The vtable slot types take void*, and calling a function + * through a function-pointer type that is not compatible with the function's + * own type is undefined behaviour (C11 6.5.2.2p9), so each slot below gets a + * thunk with the exact slot signature instead of a cast. A cast would also + * hide a genuine signature mismatch. */ +static dict_itor* vt_hashtable_dict_itor_new(void* obj) { return hashtable_dict_itor_new(obj); } +static size_t vt_hashtable_free(void* obj, dict_delete_func delete_func) { return hashtable_free(obj, delete_func); } +static dict_insert_result vt_hashtable_insert(void* obj, void* key) { return hashtable_insert(obj, key); } +static void** vt_hashtable_search(void* obj, const void* key) { return hashtable_search(obj, key); } +static dict_remove_result vt_hashtable_remove(void* obj, const void* key) { return hashtable_remove(obj, key); } +static size_t vt_hashtable_clear(void* obj, dict_delete_func delete_func) { return hashtable_clear(obj, delete_func); } +static size_t vt_hashtable_traverse(void* obj, dict_visit_func visit, void* user_data) { return hashtable_traverse(obj, visit, user_data); } +static size_t vt_hashtable_count(const void* obj) { return hashtable_count(obj); } +static bool vt_hashtable_verify(const void* obj) { return hashtable_verify(obj); } +static void vt_hashtable_itor_free(void* itor) { hashtable_itor_free(itor); } +static bool vt_hashtable_itor_valid(const void* itor) { return hashtable_itor_valid(itor); } +static void vt_hashtable_itor_invalidate(void* itor) { hashtable_itor_invalidate(itor); } +static bool vt_hashtable_itor_next(void* itor) { return hashtable_itor_next(itor); } +static bool vt_hashtable_itor_prev(void* itor) { return hashtable_itor_prev(itor); } +static bool vt_hashtable_itor_nextn(void* itor, size_t count) { return hashtable_itor_nextn(itor, count); } +static bool vt_hashtable_itor_prevn(void* itor, size_t count) { return hashtable_itor_prevn(itor, count); } +static bool vt_hashtable_itor_first(void* itor) { return hashtable_itor_first(itor); } +static bool vt_hashtable_itor_last(void* itor) { return hashtable_itor_last(itor); } +static const void* vt_hashtable_itor_key(const void* itor) { return hashtable_itor_key(itor); } +static void** vt_hashtable_itor_datum(void* itor) { return hashtable_itor_datum(itor); } +static bool vt_hashtable_itor_search(void* itor, const void* key) { return hashtable_itor_search(itor, key); } +static bool vt_hashtable_itor_remove(void* itor) { return hashtable_itor_remove(itor); } +static int vt_hashtable_itor_compare(const void* itor1, const void* itor2) { return hashtable_itor_compare(itor1, itor2); } + static const dict_vtable hashtable_vtable = { false, - (dict_inew_func) hashtable_dict_itor_new, - (dict_dfree_func) hashtable_free, - (dict_insert_func) hashtable_insert, - (dict_search_func) hashtable_search, - (dict_search_func) NULL,/* search_le: not supported */ - (dict_search_func) NULL,/* search_lt: not supported */ - (dict_search_func) NULL,/* search_ge: not supported */ - (dict_search_func) NULL,/* search_gt: not supported */ - (dict_remove_func) hashtable_remove, - (dict_clear_func) hashtable_clear, - (dict_traverse_func) hashtable_traverse, - (dict_select_func) NULL, - (dict_count_func) hashtable_count, - (dict_verify_func) hashtable_verify, + vt_hashtable_dict_itor_new, + vt_hashtable_free, + vt_hashtable_insert, + vt_hashtable_search, + NULL,/* search_le: not supported */ + NULL,/* search_lt: not supported */ + NULL,/* search_ge: not supported */ + NULL,/* search_gt: not supported */ + vt_hashtable_remove, + vt_hashtable_clear, + vt_hashtable_traverse, + NULL, + vt_hashtable_count, + vt_hashtable_verify, }; static const itor_vtable hashtable_itor_vtable = { - (dict_ifree_func) hashtable_itor_free, - (dict_valid_func) hashtable_itor_valid, - (dict_invalidate_func) hashtable_itor_invalidate, - (dict_next_func) hashtable_itor_next, - (dict_prev_func) hashtable_itor_prev, - (dict_nextn_func) hashtable_itor_nextn, - (dict_prevn_func) hashtable_itor_prevn, - (dict_first_func) hashtable_itor_first, - (dict_last_func) hashtable_itor_last, - (dict_key_func) hashtable_itor_key, - (dict_datum_func) hashtable_itor_datum, - (dict_isearch_func) hashtable_itor_search, - (dict_isearch_func) NULL,/* itor_search_le: not supported */ - (dict_isearch_func) NULL,/* itor_search_lt: not supported */ - (dict_isearch_func) NULL,/* itor_search_ge: not supported */ - (dict_isearch_func) NULL,/* itor_search_gt: not supported */ - (dict_iremove_func) hashtable_itor_remove,/* hashtable_itor_remove not implemented yet */ - (dict_icompare_func) NULL,/* hashtable_itor_compare not implemented yet */ + vt_hashtable_itor_free, + vt_hashtable_itor_valid, + vt_hashtable_itor_invalidate, + vt_hashtable_itor_next, + vt_hashtable_itor_prev, + vt_hashtable_itor_nextn, + vt_hashtable_itor_prevn, + vt_hashtable_itor_first, + vt_hashtable_itor_last, + vt_hashtable_itor_key, + vt_hashtable_itor_datum, + vt_hashtable_itor_search, + NULL,/* itor_search_le: not supported */ + NULL,/* itor_search_lt: not supported */ + NULL,/* itor_search_ge: not supported */ + NULL,/* itor_search_gt: not supported */ + vt_hashtable_itor_remove, + vt_hashtable_itor_compare, }; hashtable* @@ -117,15 +153,24 @@ hashtable_new(dict_compare_func cmp_func, dict_hash_func hash_func, unsigned siz hashtable* table = MALLOC(sizeof(*table)); if (table) { table->size = dict_prime_geq(size); - table->table = MALLOC(table->size * sizeof(hash_node*)); + /* Guard the byte count against wrapping size_t. Unreachable where + * size_t is wider than unsigned, but on ILP32 an unchecked product + * would allocate a small table and then index past it. */ + const size_t bytes = (size_t) table->size * sizeof(hash_node*); + if (bytes / sizeof(hash_node*) != (size_t) table->size) { + FREE(table); + return NULL; + } + table->table = MALLOC(bytes); if (!table->table) { FREE(table); return NULL; } - memset(table->table, 0, table->size * sizeof(hash_node*)); + memset(table->table, 0, bytes); table->cmp_func = cmp_func; table->hash_func = hash_func; table->count = 0; + table->gen = 0; } return table; } @@ -133,6 +178,7 @@ hashtable_new(dict_compare_func cmp_func, dict_hash_func hash_func, unsigned siz dict* hashtable_dict_new(dict_compare_func cmp_func, dict_hash_func hash_func, unsigned size) { + ASSERT(cmp_func != NULL); ASSERT(hash_func != NULL); ASSERT(size > 0); @@ -163,8 +209,12 @@ dict_insert_result hashtable_insert(hashtable* table, void* key) { if (LOADFACTOR_DENOMINATOR * table->count >= LOADFACTOR_NUMERATOR * table->size) { - /* Load factor too high. */ - hashtable_resize(table, table->size + 1); + /* Load factor too high: grow the table. If that fails there is no + * memory for a bigger table, but the insert can still proceed into the + * existing one -- chains simply get longer. */ + if (!hashtable_resize(table, table->size + 1)) { + /* Nothing to do; the insert below still works. */ + } } const unsigned hash = table->hash_func(key); @@ -197,6 +247,7 @@ hashtable_insert(hashtable* table, void* key) node->prev = add; table->count++; + table->gen++; return (dict_insert_result) { &add->datum, true }; } @@ -227,6 +278,7 @@ remove_node(hashtable* table, hash_node* node, unsigned mhash) FREE(node); table->count--; + table->gen++; } dict_remove_result @@ -263,6 +315,8 @@ hashtable_clear(hashtable* table, dict_delete_func delete_func) const size_t count = table->count; table->count = 0; + if (count) + table->gen++; return count; } @@ -309,11 +363,15 @@ hashtable_resize(hashtable* table, unsigned new_size) new_size = dict_prime_geq(new_size); if (table->size == new_size) return true; + /* Guard the byte count against wrapping size_t (see hashtable_new). */ + const size_t nbytes = (size_t) new_size * sizeof(hash_node*); + if (nbytes / sizeof(hash_node*) != (size_t) new_size) return false; + /* TODO: investigate whether using realloc would be advantageous. */ - hash_node** ntable = MALLOC(new_size * sizeof(hash_node*)); + hash_node** ntable = MALLOC(nbytes); if (!ntable) return false; - memset(ntable, 0, new_size * sizeof(hash_node*)); + memset(ntable, 0, nbytes); for (unsigned i = 0; i < table->size; i++) { for (hash_node* node = table->table[i]; node;) { @@ -338,6 +396,8 @@ hashtable_resize(hashtable* table, unsigned new_size) FREE(table->table); table->table = ntable; table->size = new_size; + /* Every node moved to a new bucket, so cached slots are meaningless. */ + table->gen++; return true; } @@ -370,6 +430,7 @@ hashtable_itor_new(hashtable* table) itor->table = table; itor->node = NULL; itor->slot = 0; + itor->gen = table->gen; } return itor; } @@ -394,10 +455,13 @@ hashtable_itor_free(hashtable_itor* itor) FREE(itor); } +/* True only if the iterator still designates a live element: it must be + * positioned, and the table must not have been structurally modified since the + * iterator was last positioned. */ bool hashtable_itor_valid(const hashtable_itor* itor) { - return itor->node != NULL; + return itor->node != NULL && itor->gen == itor->table->gen; } void @@ -405,12 +469,13 @@ hashtable_itor_invalidate(hashtable_itor* itor) { itor->node = NULL; itor->slot = 0; + itor->gen = itor->table->gen; } bool hashtable_itor_next(hashtable_itor* itor) { - if (!itor->node) + if (!hashtable_itor_valid(itor)) return false; if ((itor->node = itor->node->next) != NULL) @@ -426,13 +491,13 @@ hashtable_itor_next(hashtable_itor* itor) } itor->node = NULL; itor->slot = 0; - return itor->node != NULL; + return false; } bool hashtable_itor_prev(hashtable_itor* itor) { - if (!itor->node) + if (!hashtable_itor_valid(itor)) return false; if ((itor->node = itor->node->prev) != NULL) @@ -461,7 +526,7 @@ hashtable_itor_nextn(hashtable_itor* itor, size_t count) if (!hashtable_itor_next(itor)) return false; } - return itor->node != NULL; + return hashtable_itor_valid(itor); } bool @@ -471,12 +536,13 @@ hashtable_itor_prevn(hashtable_itor* itor, size_t count) if (!hashtable_itor_prev(itor)) return false; } - return itor->node != NULL; + return hashtable_itor_valid(itor); } bool hashtable_itor_first(hashtable_itor* itor) { + itor->gen = itor->table->gen; for (unsigned slot = 0; slot < itor->table->size; ++slot) { if (itor->table->table[slot]) { itor->node = itor->table->table[slot]; @@ -492,6 +558,7 @@ hashtable_itor_first(hashtable_itor* itor) bool hashtable_itor_last(hashtable_itor* itor) { + itor->gen = itor->table->gen; for (unsigned slot = itor->table->size; slot > 0;) { if (itor->table->table[--slot]) { hash_node* node = itor->table->table[slot]; @@ -510,6 +577,7 @@ hashtable_itor_last(hashtable_itor* itor) bool hashtable_itor_search(hashtable_itor* itor, const void* key) { + itor->gen = itor->table->gen; const unsigned hash = itor->table->hash_func(key); const unsigned mhash = hash % itor->table->size; hash_node* node = itor->table->table[mhash]; @@ -529,21 +597,47 @@ hashtable_itor_search(hashtable_itor* itor, const void* key) const void* hashtable_itor_key(const hashtable_itor* itor) { - return itor->node ? itor->node->key : NULL; + return hashtable_itor_valid(itor) ? itor->node->key : NULL; } void** hashtable_itor_datum(hashtable_itor* itor) { - return itor->node ? &itor->node->datum : NULL; + return hashtable_itor_valid(itor) ? &itor->node->datum : NULL; } bool hashtable_itor_remove(hashtable_itor* itor) { - if (!itor->node) + if (!hashtable_itor_valid(itor)) return false; remove_node(itor->table, itor->node, itor->node->hash % itor->table->size); itor->node = NULL; + itor->gen = itor->table->gen; return true; } + +/* A hashtable is unordered, so iterators are ordered by their position in the + * table's current traversal order (bucket index, then position within that + * bucket's chain). That order matches hashtable_itor_next()/_prev(), but it is + * only meaningful until the next structural modification of the table. */ +int +hashtable_itor_compare(const hashtable_itor* i1, const hashtable_itor* i2) +{ + ASSERT(i1->table == i2->table); + + const bool v1 = hashtable_itor_valid(i1), v2 = hashtable_itor_valid(i2); + if (!v1) + return !v2 ? 0 : -1; + if (!v2) + return 1; + if (i1->node == i2->node) + return 0; + if (i1->slot != i2->slot) + return i1->slot < i2->slot ? -1 : 1; + /* Same bucket: whichever node comes first in the chain compares less. */ + for (const hash_node* n = i1->node->next; n; n = n->next) + if (n == i2->node) + return -1; + return 1; +} diff --git a/src/hashtable2.c b/src/hashtable2.c index 580d1c69..cf14c9e1 100644 --- a/src/hashtable2.c +++ b/src/hashtable2.c @@ -1,5 +1,5 @@ /* - * libdict -- chained hash-table, with chains sorted by hash, implementation. + * libdict -- open-addressing hash-table implementation. * * Copyright (c) 2001-2014, Farooq Mela * All rights reserved. @@ -56,50 +56,87 @@ struct hashtable2 { dict_hash_func hash_func; hash_node* table; unsigned size; + /* Bumped by every structural modification. Iterators record it so they can + * detect that the slot they cached no longer holds the element they were + * positioned on -- removals both empty slots and relocate other elements + * via the backward shift in remove_cleanup(). */ + unsigned gen; }; struct hashtable2_itor { hashtable2* table; - int slot; + /* Negative means "not positioned"; otherwise an index into table->table. + * Signed 64-bit so that every unsigned slot index is representable. */ + ptrdiff_t slot; + unsigned gen; }; +/* Vtable thunks. The vtable slot types take void*, and calling a function + * through a function-pointer type that is not compatible with the function's + * own type is undefined behaviour (C11 6.5.2.2p9), so each slot below gets a + * thunk with the exact slot signature instead of a cast. A cast would also + * hide a genuine signature mismatch. */ +static dict_itor* vt_hashtable2_dict_itor_new(void* obj) { return hashtable2_dict_itor_new(obj); } +static size_t vt_hashtable2_free(void* obj, dict_delete_func delete_func) { return hashtable2_free(obj, delete_func); } +static dict_insert_result vt_hashtable2_insert(void* obj, void* key) { return hashtable2_insert(obj, key); } +static void** vt_hashtable2_search(void* obj, const void* key) { return hashtable2_search(obj, key); } +static dict_remove_result vt_hashtable2_remove(void* obj, const void* key) { return hashtable2_remove(obj, key); } +static size_t vt_hashtable2_clear(void* obj, dict_delete_func delete_func) { return hashtable2_clear(obj, delete_func); } +static size_t vt_hashtable2_traverse(void* obj, dict_visit_func visit, void* user_data) { return hashtable2_traverse(obj, visit, user_data); } +static size_t vt_hashtable2_count(const void* obj) { return hashtable2_count(obj); } +static bool vt_hashtable2_verify(const void* obj) { return hashtable2_verify(obj); } +static void vt_hashtable2_itor_free(void* itor) { hashtable2_itor_free(itor); } +static bool vt_hashtable2_itor_valid(const void* itor) { return hashtable2_itor_valid(itor); } +static void vt_hashtable2_itor_invalidate(void* itor) { hashtable2_itor_invalidate(itor); } +static bool vt_hashtable2_itor_next(void* itor) { return hashtable2_itor_next(itor); } +static bool vt_hashtable2_itor_prev(void* itor) { return hashtable2_itor_prev(itor); } +static bool vt_hashtable2_itor_nextn(void* itor, size_t count) { return hashtable2_itor_nextn(itor, count); } +static bool vt_hashtable2_itor_prevn(void* itor, size_t count) { return hashtable2_itor_prevn(itor, count); } +static bool vt_hashtable2_itor_first(void* itor) { return hashtable2_itor_first(itor); } +static bool vt_hashtable2_itor_last(void* itor) { return hashtable2_itor_last(itor); } +static const void* vt_hashtable2_itor_key(const void* itor) { return hashtable2_itor_key(itor); } +static void** vt_hashtable2_itor_datum(void* itor) { return hashtable2_itor_datum(itor); } +static bool vt_hashtable2_itor_search(void* itor, const void* key) { return hashtable2_itor_search(itor, key); } +static bool vt_hashtable2_itor_remove(void* itor) { return hashtable2_itor_remove(itor); } +static int vt_hashtable2_itor_compare(const void* itor1, const void* itor2) { return hashtable2_itor_compare(itor1, itor2); } + static const dict_vtable hashtable2_vtable = { false, - (dict_inew_func) hashtable2_dict_itor_new, - (dict_dfree_func) hashtable2_free, - (dict_insert_func) hashtable2_insert, - (dict_search_func) hashtable2_search, - (dict_search_func) NULL,/* search_le: not supported */ - (dict_search_func) NULL,/* search_lt: not supported */ - (dict_search_func) NULL,/* search_ge: not supported */ - (dict_search_func) NULL,/* search_gt: not supported */ - (dict_remove_func) hashtable2_remove, - (dict_clear_func) hashtable2_clear, - (dict_traverse_func) hashtable2_traverse, - (dict_select_func) NULL, - (dict_count_func) hashtable2_count, - (dict_verify_func) hashtable2_verify, + vt_hashtable2_dict_itor_new, + vt_hashtable2_free, + vt_hashtable2_insert, + vt_hashtable2_search, + NULL,/* search_le: not supported */ + NULL,/* search_lt: not supported */ + NULL,/* search_ge: not supported */ + NULL,/* search_gt: not supported */ + vt_hashtable2_remove, + vt_hashtable2_clear, + vt_hashtable2_traverse, + NULL, + vt_hashtable2_count, + vt_hashtable2_verify, }; static const itor_vtable hashtable2_itor_vtable = { - (dict_ifree_func) hashtable2_itor_free, - (dict_valid_func) hashtable2_itor_valid, - (dict_invalidate_func) hashtable2_itor_invalidate, - (dict_next_func) hashtable2_itor_next, - (dict_prev_func) hashtable2_itor_prev, - (dict_nextn_func) hashtable2_itor_nextn, - (dict_prevn_func) hashtable2_itor_prevn, - (dict_first_func) hashtable2_itor_first, - (dict_last_func) hashtable2_itor_last, - (dict_key_func) hashtable2_itor_key, - (dict_datum_func) hashtable2_itor_datum, - (dict_isearch_func) hashtable2_itor_search, - (dict_isearch_func) NULL,/* itor_search_le: not supported */ - (dict_isearch_func) NULL,/* itor_search_lt: not supported */ - (dict_isearch_func) NULL,/* itor_search_ge: not supported */ - (dict_isearch_func) NULL,/* itor_search_gt: not supported */ - (dict_iremove_func) hashtable2_itor_remove, - (dict_icompare_func) NULL,/* hashtable2_itor_compare not implemented yet */ + vt_hashtable2_itor_free, + vt_hashtable2_itor_valid, + vt_hashtable2_itor_invalidate, + vt_hashtable2_itor_next, + vt_hashtable2_itor_prev, + vt_hashtable2_itor_nextn, + vt_hashtable2_itor_prevn, + vt_hashtable2_itor_first, + vt_hashtable2_itor_last, + vt_hashtable2_itor_key, + vt_hashtable2_itor_datum, + vt_hashtable2_itor_search, + NULL,/* itor_search_le: not supported */ + NULL,/* itor_search_lt: not supported */ + NULL,/* itor_search_ge: not supported */ + NULL,/* itor_search_gt: not supported */ + vt_hashtable2_itor_remove, + vt_hashtable2_itor_compare, }; hashtable2* @@ -112,15 +149,24 @@ hashtable2_new(dict_compare_func cmp_func, dict_hash_func hash_func, unsigned in hashtable2* table = MALLOC(sizeof(*table)); if (table) { table->size = dict_prime_geq(initial_size); - table->table = MALLOC(table->size * sizeof(hash_node)); + /* Guard the byte count against wrapping size_t. Unreachable where + * size_t is wider than unsigned, but on ILP32 an unchecked product + * would allocate a small table and then index past it. */ + const size_t bytes = (size_t) table->size * sizeof(hash_node); + if (bytes / sizeof(hash_node) != (size_t) table->size) { + FREE(table); + return NULL; + } + table->table = MALLOC(bytes); if (!table->table) { FREE(table); return NULL; } - memset(table->table, 0, table->size * sizeof(hash_node)); + memset(table->table, 0, bytes); table->cmp_func = cmp_func; table->hash_func = hash_func; table->count = 0; + table->gen = 0; } return table; } @@ -128,6 +174,7 @@ hashtable2_new(dict_compare_func cmp_func, dict_hash_func hash_func, unsigned in dict* hashtable2_dict_new(dict_compare_func cmp_func, dict_hash_func hash_func, unsigned initial_size) { + ASSERT(cmp_func != NULL); ASSERT(hash_func != NULL); ASSERT(initial_size > 0); @@ -146,6 +193,8 @@ hashtable2_dict_new(dict_compare_func cmp_func, dict_hash_func hash_func, unsign size_t hashtable2_free(hashtable2* table, dict_delete_func delete_func) { + ASSERT(table != NULL); + size_t count = hashtable2_clear(table, delete_func); FREE(table->table); FREE(table); @@ -194,8 +243,10 @@ hashtable2_insert(hashtable2* table, void* key) } const unsigned hash = nonzero_hash(table->hash_func, key); dict_insert_result result = insert(table, key, hash); - if (result.inserted) + if (result.inserted) { table->count++; + table->gen++; + } return result; } @@ -219,27 +270,6 @@ hashtable2_search(hashtable2* table, const void* key) return NULL; } -#if 0 -static int -index_of_node_to_shift(hashtable2* table, unsigned truncated_hash, unsigned index) -{ - int last_index = -1; - do { - hash_node* node = &table->table[index]; - if (!node->hash) { - break; - } - if (node->hash % table->size == truncated_hash) { - last_index = index; - } - if (++index == table->size) { - index = 0; - } - } while (index != truncated_hash); - return last_index; -} -#endif - static void remove_cleanup(hashtable2* table, hash_node* const first, hash_node* node) { @@ -272,6 +302,7 @@ remove_node(hashtable2* table, hash_node* first, hash_node* node) node->key = node->datum = NULL; node->hash = 0; table->count--; + table->gen++; if (++node == table->table + table->size) node = table->table; @@ -316,6 +347,8 @@ hashtable2_clear(hashtable2* table, dict_delete_func delete_func) const size_t count = table->count; table->count = 0; + if (count) + table->gen++; return count; } @@ -365,16 +398,21 @@ hashtable2_resize(hashtable2* table, unsigned new_size) return false; } + /* Guard the byte count against wrapping size_t (see hashtable2_new). */ + const size_t nbytes = (size_t) new_size * sizeof(hash_node); + if (nbytes / sizeof(hash_node) != (size_t) new_size) + return false; + const unsigned old_size = table->size; const size_t old_count = table->count; hash_node* const old_table = table->table; - table->table = MALLOC(new_size * sizeof(hash_node)); + table->table = MALLOC(nbytes); if (!table->table) { table->table = old_table; return false; } - memset(table->table, 0, new_size * sizeof(hash_node)); + memset(table->table, 0, nbytes); table->size = new_size; for (unsigned i = 0; i < old_size; ++i) { @@ -392,6 +430,8 @@ hashtable2_resize(hashtable2* table, unsigned new_size) } } FREE(old_table); + /* Every element was re-homed, so cached slots are meaningless. */ + table->gen++; return true; } @@ -419,6 +459,7 @@ hashtable2_itor_new(hashtable2* table) if (itor) { itor->table = table; itor->slot = -1; + itor->gen = table->gen; } return itor; } @@ -443,28 +484,31 @@ hashtable2_itor_free(hashtable2_itor* itor) FREE(itor); } +/* True only if the iterator still designates a live element. The slot must be + * re-checked rather than assumed occupied: a removal can empty it, and + * remove_cleanup()'s backward shift can move a *different* element into it. */ bool hashtable2_itor_valid(const hashtable2_itor* itor) { - if (itor->slot < 0) - return false; - ASSERT(itor->table->table[itor->slot].hash != 0); - return true; + return itor->slot >= 0 + && itor->gen == itor->table->gen + && itor->table->table[itor->slot].hash != 0; } void hashtable2_itor_invalidate(hashtable2_itor* itor) { itor->slot = -1; + itor->gen = itor->table->gen; } bool hashtable2_itor_next(hashtable2_itor* itor) { - if (itor->slot < 0) + if (!hashtable2_itor_valid(itor)) return false; - while (++itor->slot < (int) itor->table->size) { + while (++itor->slot < (ptrdiff_t) itor->table->size) { if (itor->table->table[itor->slot].hash) return true; } @@ -475,7 +519,7 @@ hashtable2_itor_next(hashtable2_itor* itor) bool hashtable2_itor_prev(hashtable2_itor* itor) { - if (itor->slot < 0) + if (!hashtable2_itor_valid(itor)) return false; while (itor->slot-- > 0) { @@ -492,7 +536,7 @@ hashtable2_itor_nextn(hashtable2_itor* itor, size_t count) while (count--) if (!hashtable2_itor_next(itor)) return false; - return itor->slot >= 0; + return hashtable2_itor_valid(itor); } bool @@ -501,15 +545,16 @@ hashtable2_itor_prevn(hashtable2_itor* itor, size_t count) while (count--) if (!hashtable2_itor_prev(itor)) return false; - return itor->slot >= 0; + return hashtable2_itor_valid(itor); } bool hashtable2_itor_first(hashtable2_itor* itor) { + itor->gen = itor->table->gen; for (unsigned slot = 0; slot < itor->table->size; ++slot) { if (itor->table->table[slot].hash) { - itor->slot = (int) slot; + itor->slot = (ptrdiff_t) slot; return true; } } @@ -520,9 +565,10 @@ hashtable2_itor_first(hashtable2_itor* itor) bool hashtable2_itor_last(hashtable2_itor* itor) { + itor->gen = itor->table->gen; for (unsigned slot = itor->table->size; slot > 0;) { if (itor->table->table[--slot].hash) { - itor->slot = (int) slot; + itor->slot = (ptrdiff_t) slot; return true; } } @@ -533,6 +579,7 @@ hashtable2_itor_last(hashtable2_itor* itor) bool hashtable2_itor_search(hashtable2_itor* itor, const void* key) { + itor->gen = itor->table->gen; const unsigned hash = nonzero_hash(itor->table->hash_func, key); const unsigned truncated_hash = hash % itor->table->size; unsigned index = truncated_hash; @@ -541,37 +588,54 @@ hashtable2_itor_search(hashtable2_itor* itor, const void* key) if (!node->hash) break; if (node->hash == hash && itor->table->cmp_func(key, node->key) == 0) { - itor->slot = (int) index; + itor->slot = (ptrdiff_t) index; return true; } if (++index == itor->table->size) index = 0; } while (index != truncated_hash); itor->slot = -1; - return NULL; + return false; } const void* hashtable2_itor_key(const hashtable2_itor* itor) { - return (itor->slot >= 0) ? itor->table->table[itor->slot].key : NULL; + return hashtable2_itor_valid(itor) ? itor->table->table[itor->slot].key : NULL; } void** hashtable2_itor_datum(hashtable2_itor* itor) { - return (itor->slot >= 0) ? &itor->table->table[itor->slot].datum : NULL; + return hashtable2_itor_valid(itor) ? &itor->table->table[itor->slot].datum : NULL; } bool hashtable2_itor_remove(hashtable2_itor* itor) { - if (itor->slot < 0) + if (!hashtable2_itor_valid(itor)) return false; remove_node(itor->table, itor->table->table + itor->table->table[itor->slot].hash % itor->table->size, itor->table->table + itor->slot); itor->slot = -1; + itor->gen = itor->table->gen; return true; } +/* A hashtable is unordered, so iterators are ordered by slot index, which is + * the order hashtable2_itor_next() walks. That order is only meaningful until + * the next structural modification of the table. */ +int +hashtable2_itor_compare(const hashtable2_itor* i1, const hashtable2_itor* i2) +{ + ASSERT(i1->table == i2->table); + + const bool v1 = hashtable2_itor_valid(i1), v2 = hashtable2_itor_valid(i2); + if (!v1) + return !v2 ? 0 : -1; + if (!v2) + return 1; + return (i1->slot > i2->slot) - (i1->slot < i2->slot); +} + diff --git a/src/hashtable_common.h b/src/hashtable_common.h index 23fac3c0..a6e0db39 100644 --- a/src/hashtable_common.h +++ b/src/hashtable_common.h @@ -28,6 +28,10 @@ #ifndef LIBDICT_HASHTABLE_COMMON_H__ #define LIBDICT_HASHTABLE_COMMON_H__ +/* Returns the smallest prime from an internal table that is >= |n|, or the + * largest prime in the table (4294967291) if |n| exceeds it. Callers that + * allocate |result| elements must therefore check the result against |n|, or + * guard the allocation size themselves -- see the *_resize() functions. */ unsigned dict_prime_geq(unsigned n); #endif /* !LIBDICT_HASHTABLE_COMMON_H__ */ diff --git a/src/hb_tree.c b/src/hb_tree.c index d47dfb45..e120b1fc 100644 --- a/src/hb_tree.c +++ b/src/hb_tree.c @@ -48,6 +48,26 @@ struct hb_node { #define BAL_POS(node) ((node)->bal & 1) #define BAL_NEG(node) ((node)->bal & 2) +/* hb_node is handed to the generic routines in tree_common.c as though it were + * a tree_node_base, so its first five members must sit at the same offsets. + * The third of those, the parent pointer, additionally carries the balance + * factor in its two low bits, so ONLY this file's PARENT() macro may decode + * it: any tree_common routine that reads |parent| directly (tree_traverse, + * tree_select, tree_clear, tree_iterator_next/prev, tree_node_rot_*) would + * dereference a tagged, misaligned pointer and must be reimplemented here. */ +_Static_assert(sizeof(intptr_t) == sizeof(hb_node*), + "hb_node tags a parent pointer stored in an intptr_t"); +_Static_assert(offsetof(hb_node, key) == offsetof(tree_node_base, key), + "hb_node must be layout-compatible with tree_node_base"); +_Static_assert(offsetof(hb_node, datum) == offsetof(tree_node_base, datum), + "hb_node must be layout-compatible with tree_node_base"); +_Static_assert(offsetof(hb_node, bal) == offsetof(tree_node_base, parent), + "hb_node must be layout-compatible with tree_node_base"); +_Static_assert(offsetof(hb_node, llink) == offsetof(tree_node_base, llink), + "hb_node must be layout-compatible with tree_node_base"); +_Static_assert(offsetof(hb_node, rlink) == offsetof(tree_node_base, rlink), + "hb_node must be layout-compatible with tree_node_base"); + struct hb_tree { TREE_FIELDS(hb_node); }; @@ -56,43 +76,62 @@ struct hb_itor { TREE_ITERATOR_FIELDS(hb_tree, hb_node); }; +/* Vtable thunks. The vtable slot types take void*, and calling a function + * through a function-pointer type that is not compatible with the function's + * own type is undefined behaviour (C11 6.5.2.2p9), so each slot below gets a + * thunk with the exact slot signature instead of a cast. A cast would also + * hide a genuine signature mismatch. */ +static dict_itor* vt_hb_dict_itor_new(void* obj) { return hb_dict_itor_new(obj); } +static size_t vt_hb_tree_free(void* obj, dict_delete_func delete_func) { return hb_tree_free(obj, delete_func); } +static dict_insert_result vt_hb_tree_insert(void* obj, void* key) { return hb_tree_insert(obj, key); } +static dict_remove_result vt_hb_tree_remove(void* obj, const void* key) { return hb_tree_remove(obj, key); } +static size_t vt_hb_tree_clear(void* obj, dict_delete_func delete_func) { return hb_tree_clear(obj, delete_func); } +static size_t vt_hb_tree_traverse(void* obj, dict_visit_func visit, void* user_data) { return hb_tree_traverse(obj, visit, user_data); } +static bool vt_hb_tree_select(void* obj, size_t n, const void** key, void** datum) { return hb_tree_select(obj, n, key, datum); } +static bool vt_hb_tree_verify(const void* obj) { return hb_tree_verify(obj); } +static bool vt_hb_itor_next(void* itor) { return hb_itor_next(itor); } +static bool vt_hb_itor_prev(void* itor) { return hb_itor_prev(itor); } +static bool vt_hb_itor_nextn(void* itor, size_t count) { return hb_itor_nextn(itor, count); } +static bool vt_hb_itor_prevn(void* itor, size_t count) { return hb_itor_prevn(itor, count); } +static bool vt_hb_itor_remove(void* itor) { return hb_itor_remove(itor); } + static const dict_vtable hb_tree_vtable = { true, - (dict_inew_func) hb_dict_itor_new, - (dict_dfree_func) hb_tree_free, - (dict_insert_func) hb_tree_insert, - (dict_search_func) tree_search, - (dict_search_func) tree_search_le, - (dict_search_func) tree_search_lt, - (dict_search_func) tree_search_ge, - (dict_search_func) tree_search_gt, - (dict_remove_func) hb_tree_remove, - (dict_clear_func) hb_tree_clear, - (dict_traverse_func) hb_tree_traverse, - (dict_select_func) hb_tree_select, - (dict_count_func) tree_count, - (dict_verify_func) hb_tree_verify, + vt_hb_dict_itor_new, + vt_hb_tree_free, + vt_hb_tree_insert, + tree_search, + tree_search_le, + tree_search_lt, + tree_search_ge, + tree_search_gt, + vt_hb_tree_remove, + vt_hb_tree_clear, + vt_hb_tree_traverse, + vt_hb_tree_select, + tree_count, + vt_hb_tree_verify, }; static const itor_vtable hb_tree_itor_vtable = { - (dict_ifree_func) tree_iterator_free, - (dict_valid_func) tree_iterator_valid, - (dict_invalidate_func) tree_iterator_invalidate, - (dict_next_func) hb_itor_next, - (dict_prev_func) hb_itor_prev, - (dict_nextn_func) hb_itor_nextn, - (dict_prevn_func) hb_itor_prevn, - (dict_first_func) tree_iterator_first, - (dict_last_func) tree_iterator_last, - (dict_key_func) tree_iterator_key, - (dict_datum_func) tree_iterator_datum, - (dict_isearch_func) tree_iterator_search, - (dict_isearch_func) tree_iterator_search_le, - (dict_isearch_func) tree_iterator_search_lt, - (dict_isearch_func) tree_iterator_search_ge, - (dict_isearch_func) tree_iterator_search_gt, - (dict_iremove_func) hb_itor_remove, - (dict_icompare_func) tree_iterator_compare, + tree_iterator_free, + tree_iterator_valid, + tree_iterator_invalidate, + vt_hb_itor_next, + vt_hb_itor_prev, + vt_hb_itor_nextn, + vt_hb_itor_prevn, + tree_iterator_first, + tree_iterator_last, + tree_iterator_key, + tree_iterator_datum, + tree_iterator_search, + tree_iterator_search_le, + tree_iterator_search_lt, + tree_iterator_search_ge, + tree_iterator_search_gt, + vt_hb_itor_remove, + tree_iterator_compare, }; static hb_node* node_prev(hb_node* node); @@ -100,7 +139,8 @@ static hb_node* node_next(hb_node* node); static hb_node* node_new(void* key); static bool node_verify(const hb_tree* tree, const hb_node* parent, const hb_node* node, - unsigned* height, size_t *count); + unsigned* height, size_t *count, + const void* lower, const void* upper); hb_tree* hb_tree_new(dict_compare_func cmp_func) @@ -151,8 +191,9 @@ hb_tree_clear(hb_tree* tree, dict_delete_func delete_func) } if (delete_func) delete_func(node->key, node->datum); hb_node* const parent = PARENT(node); - FREE(node); + /* Unlink before freeing: comparing a pointer to a freed object is UB. */ *(parent ? (parent->llink == node ? &parent->llink : &parent->rlink) : &tree->root) = NULL; + FREE(node); node = parent; } ASSERT(tree->root == NULL); @@ -373,6 +414,10 @@ remove_node(hb_tree* tree, hb_node* node) hb_node* p = PARENT(node); hb_node* child = node->llink ? node->llink : node->rlink; + /* Decide which side of |p| the node hangs off, and relink, BEFORE freeing: + * comparing a pointer to a freed object is UB. */ + bool left = p && (p->llink == node); + ASSERT(!p || left || p->rlink == node); FREE(node); tree->count--; if (child) @@ -382,11 +427,9 @@ remove_node(hb_tree* tree, hb_node* node) return; } - bool left = (p->llink == node); if (left) { p->llink = child; } else { - ASSERT(p->rlink == node); p->rlink = child; } node = child; @@ -460,7 +503,18 @@ hb_tree_remove(hb_tree* tree, const void* key) size_t hb_tree_traverse(hb_tree* tree, dict_visit_func visit, void* user_data) { - return tree_traverse(tree, visit, user_data); + ASSERT(visit != NULL); + + /* NOTE: this must NOT delegate to the generic tree_traverse(). hb_node + * packs the balance factor into the low bits of its parent pointer, so + * only node_next()/PARENT() may walk the parent chain here. */ + size_t count = 0; + for (hb_node* node = tree_node_min(tree->root); node; node = node_next(node)) { + ++count; + if (!visit(node->key, node->datum, user_data)) + break; + } + return count; } bool @@ -501,7 +555,16 @@ node_new(void* key) { hb_node* node = MALLOC(sizeof(*node)); if (node) { - ASSERT((((intptr_t)node) & 3) == 0); /* Ensure malloc returns aligned result. */ + /* The two low bits of the parent pointer hold the balance factor, so + * a node must be at least 4-byte aligned. dict_malloc_func is + * user-replaceable, and an ASSERT would vanish under NDEBUG and let a + * misaligned allocator corrupt every parent pointer silently. */ + if ((((uintptr_t) node) & 3) != 0) { + fprintf(stderr, "%s:%d (%s) dict_malloc_func returned a pointer " + "that is not 4-byte aligned: %p\n", + __FILE__, __LINE__, __func__, (void*) node); + abort(); + } node->key = key; node->datum = NULL; node->bal = 0; /* also initializes parent to NULL */ @@ -539,21 +602,25 @@ node_next(hb_node* node) static bool node_verify(const hb_tree* tree, const hb_node* parent, const hb_node* node, - unsigned* height, size_t *count) + unsigned* height, size_t *count, const void* lower, const void* upper) { if (!parent) { VERIFY(tree->root == node); } else { - if (parent->llink == node) { - if (node) - VERIFY(tree->cmp_func(parent->key, node->key) > 0); - } else { - ASSERT(parent->rlink == node); - if (node) - VERIFY(tree->cmp_func(parent->key, node->key) < 0); - } + /* Was an ASSERT, which vanishes under NDEBUG and then silently + * accepted a node whose claimed parent had no link back to it. */ + VERIFY(parent->llink == node || parent->rlink == node); } if (node) { + /* Global BST ordering. Comparing a node only against its immediate + * parent would accept an entire subtree placed on the wrong side: + * root 5, 5->llink 3, 3->rlink 7 passes every parent-local check yet + * makes key 7 unreachable from the root. */ + if (lower) + VERIFY(tree->cmp_func(lower, node->key) < 0); + if (upper) + VERIFY(tree->cmp_func(upper, node->key) > 0); + int bal = node->bal & BAL_MASK; VERIFY(bal >= 0); VERIFY(bal <= 2); @@ -565,8 +632,8 @@ node_verify(const hb_tree* tree, const hb_node* parent, const hb_node* node, } VERIFY(PARENT(node) == parent); unsigned lheight, rheight; - if (!node_verify(tree, node, node->llink, &lheight, count) || - !node_verify(tree, node, node->rlink, &rheight, count)) + if (!node_verify(tree, node, node->llink, &lheight, count, lower, node->key) || + !node_verify(tree, node, node->rlink, &rheight, count, node->key, upper)) return false; VERIFY(bal == (int)rheight - (int)lheight); if (height) @@ -583,9 +650,13 @@ bool hb_tree_verify(const hb_tree* tree) { size_t count = 0; - bool verified = node_verify(tree, NULL, tree->root, NULL, &count); + /* Only check the tally if the structure itself verified: otherwise + * node_verify() bailed out with a partial count and the count mismatch + * would mask the real failure it already reported. */ + if (!node_verify(tree, NULL, tree->root, NULL, &count, NULL, NULL)) + return false; VERIFY(tree->count == count); - return verified; + return true; } hb_itor* @@ -643,7 +714,7 @@ bool hb_itor_prevn(hb_itor* itor, size_t count) { bool hb_itor_first(hb_itor* itor) { return tree_iterator_first(itor); } bool hb_itor_last(hb_itor* itor) { return tree_iterator_last(itor); } -bool hb_itor_search(hb_itor* itor, const void* key) { return tree_iterator_search_ge(itor, key); } +bool hb_itor_search(hb_itor* itor, const void* key) { return tree_iterator_search(itor, key); } bool hb_itor_search_le(hb_itor* itor, const void* key) { return tree_iterator_search_le(itor, key); } bool hb_itor_search_lt(hb_itor* itor, const void* key) { return tree_iterator_search_lt(itor, key); } bool hb_itor_search_ge(hb_itor* itor, const void* key) { return tree_iterator_search_ge(itor, key); } diff --git a/src/pr_tree.c b/src/pr_tree.c index 24979f36..9f32ad14 100644 --- a/src/pr_tree.c +++ b/src/pr_tree.c @@ -50,43 +50,55 @@ struct pr_itor { TREE_ITERATOR_FIELDS(pr_tree, pr_node); }; +/* Vtable thunks. The vtable slot types take void*, and calling a function + * through a function-pointer type that is not compatible with the function's + * own type is undefined behaviour (C11 6.5.2.2p9), so each slot below gets a + * thunk with the exact slot signature instead of a cast. A cast would also + * hide a genuine signature mismatch. */ +static dict_itor* vt_pr_dict_itor_new(void* obj) { return pr_dict_itor_new(obj); } +static dict_insert_result vt_pr_tree_insert(void* obj, void* key) { return pr_tree_insert(obj, key); } +static dict_remove_result vt_pr_tree_remove(void* obj, const void* key) { return pr_tree_remove(obj, key); } +static bool vt_pr_tree_select(void* obj, size_t n, const void** key, void** datum) { return pr_tree_select(obj, n, key, datum); } +static bool vt_pr_tree_verify(const void* obj) { return pr_tree_verify(obj); } +static bool vt_pr_itor_remove(void* itor) { return pr_itor_remove(itor); } + static const dict_vtable pr_tree_vtable = { true, - (dict_inew_func) pr_dict_itor_new, - (dict_dfree_func) tree_free, - (dict_insert_func) pr_tree_insert, - (dict_search_func) tree_search, - (dict_search_func) tree_search_le, - (dict_search_func) tree_search_lt, - (dict_search_func) tree_search_ge, - (dict_search_func) tree_search_gt, - (dict_remove_func) pr_tree_remove, - (dict_clear_func) tree_clear, - (dict_traverse_func) tree_traverse, - (dict_select_func) pr_tree_select, - (dict_count_func) tree_count, - (dict_verify_func) pr_tree_verify, + vt_pr_dict_itor_new, + tree_free, + vt_pr_tree_insert, + tree_search, + tree_search_le, + tree_search_lt, + tree_search_ge, + tree_search_gt, + vt_pr_tree_remove, + tree_clear, + tree_traverse, + vt_pr_tree_select, + tree_count, + vt_pr_tree_verify, }; static const itor_vtable pr_tree_itor_vtable = { - (dict_ifree_func) tree_iterator_free, - (dict_valid_func) tree_iterator_valid, - (dict_invalidate_func) tree_iterator_invalidate, - (dict_next_func) tree_iterator_next, - (dict_prev_func) tree_iterator_prev, - (dict_nextn_func) tree_iterator_nextn, - (dict_prevn_func) tree_iterator_prevn, - (dict_first_func) tree_iterator_first, - (dict_last_func) tree_iterator_last, - (dict_key_func) tree_iterator_key, - (dict_datum_func) tree_iterator_datum, - (dict_isearch_func) tree_iterator_search, - (dict_isearch_func) tree_iterator_search_le, - (dict_isearch_func) tree_iterator_search_lt, - (dict_isearch_func) tree_iterator_search_ge, - (dict_isearch_func) tree_iterator_search_gt, - (dict_iremove_func) pr_itor_remove, - (dict_icompare_func) tree_iterator_compare, + tree_iterator_free, + tree_iterator_valid, + tree_iterator_invalidate, + tree_iterator_next, + tree_iterator_prev, + tree_iterator_nextn, + tree_iterator_prevn, + tree_iterator_first, + tree_iterator_last, + tree_iterator_key, + tree_iterator_datum, + tree_iterator_search, + tree_iterator_search_le, + tree_iterator_search_lt, + tree_iterator_search_ge, + tree_iterator_search_gt, + vt_pr_itor_remove, + tree_iterator_compare, }; static unsigned fixup(pr_tree* tree, pr_node* node); @@ -143,23 +155,22 @@ fixup(pr_tree* tree, pr_node* node) * * Consider a node N. * A single left rotation is performed when - * WEIGHT(n->llink) < WEIGHT(n->rlink->rlink) + * WEIGHT(n->rlink->rlink) > WEIGHT(n->llink) * A right-left rotation is performed when - * WEIGHT(n->llink) < WEIGHT(n->rlink->llink) + * WEIGHT(n->rlink->llink) > WEIGHT(n->llink) * * A single right rotation is performed when - * WEIGHT(n->rlink) > WEIGHT(n->llink->llink) + * WEIGHT(n->llink->llink) > WEIGHT(n->rlink) * A left-right rotation is performed when - * WEIGHT(n->rlink) > WEIGHT(n->llink->rlink) + * WEIGHT(n->llink->rlink) > WEIGHT(n->rlink) * * Although the worst case number of rotations for a single insertion or * deletion is O(n), the amortized worst-case number of rotations is * .44042lg(n) + O(1) for insertion, and .42062lg(n) + O(1) for deletion. * - * We use tail recursion to minimize the number of recursive calls. For - * single rotations, no recursive call is made, and we tail recurse to - * continue checking for out-of-balance conditions. For double, we make one - * recursive call and then tail recurse. + * Every rotating branch below recurses twice, once into each node whose + * subtree changed shape, so the recursion depth is bounded by the height + * of the tree. */ unsigned rotations = 0; const unsigned lweight = WEIGHT(node->llink); @@ -167,7 +178,7 @@ fixup(pr_tree* tree, pr_node* node) if (lweight < rweight) { pr_node* r = node->rlink; ASSERT(r != NULL); - if (WEIGHT(r->rlink) > lweight) { /* LL */ + if (WEIGHT(r->rlink) > lweight) { /* RR */ rot_left(tree, node); rotations += 1; rotations += fixup(tree, node); @@ -200,18 +211,20 @@ fixup(pr_tree* tree, pr_node* node) r->weight += WEIGHT(b) - rl->weight; rl->weight = node->weight + r->weight; - rotations += 1; + /* A double rotation counts as two, matching the mirrored LR case + * below. */ + rotations += 2; rotations += fixup(tree, r); rotations += fixup(tree, node); } } else if (lweight > rweight) { pr_node* l = node->llink; ASSERT(l != NULL); - if (WEIGHT(l->llink) > rweight) { /* RR */ + if (WEIGHT(l->llink) > rweight) { /* LL */ rot_right(tree, node); rotations += 1; rotations += fixup(tree, node); - /* Commenting the next line means we must turn the weight + /* Commenting the next line means we must make the weight * verification condition in node_verify() less stringent. */ rotations += fixup(tree, l); } else if (WEIGHT(l->rlink) > rweight) { /* LR */ @@ -305,7 +318,7 @@ remove_node(pr_tree* tree, pr_node* node) node = out; } ASSERT(!node->llink || !node->rlink); - /* Splice in the successor, if any. */ + /* Splice in the node's only child, if any. */ pr_node* child = node->llink ? node->llink : node->rlink; pr_node* parent = node->parent; if (child) @@ -395,14 +408,14 @@ node_new(void* key) /* * rot_left(T, B): * - * / / - * B D - * / \ / \ - * A D ==> B E - * / \ / \ - * C E A C + * / / + * B D + * / \ / \ + * A D ==> B E + * / \ / \ + * C E A C * - * Only the weights of B and B's right child to be readjusted. + * Only the weights of B and B's new parent D need to be readjusted. */ static void rot_left(pr_tree* tree, pr_node *node) @@ -419,14 +432,14 @@ rot_left(pr_tree* tree, pr_node *node) /* * rot_right(T, D): * - * / / - * D B - * / \ / \ - * B E ==> A D - * / \ / \ - * A C C E + * / / + * D B + * / \ / \ + * B E ==> A D + * / \ / \ + * A C C E * - * Only the weights of D and D's left child need to be readjusted. + * Only the weights of D and D's new parent B need to be readjusted. */ static void rot_right(pr_tree* tree, pr_node* node) @@ -441,7 +454,8 @@ rot_right(pr_tree* tree, pr_node* node) } static bool -node_verify(const pr_tree* tree, const pr_node* parent, const pr_node* node) +node_verify(const pr_tree* tree, const pr_node* parent, const pr_node* node, + const void* lower, const void* upper, size_t* count) { if (!parent) { VERIFY(tree->root == node); @@ -450,18 +464,19 @@ node_verify(const pr_tree* tree, const pr_node* parent, const pr_node* node) } if (node) { VERIFY(node->parent == parent); - if (parent) { - if (parent->llink == node) { - VERIFY(tree->cmp_func(parent->key, node->key) > 0); - } else { - ASSERT(parent->rlink == node); - VERIFY(tree->cmp_func(parent->key, node->key) < 0); - } - } + /* Global BST ordering. Comparing a node only against its immediate + * parent would accept an entire subtree placed on the wrong side: + * root 5, 5->llink 3, 3->rlink 7 passes every parent-local check yet + * makes key 7 unreachable from the root. */ + if (lower) + VERIFY(tree->cmp_func(lower, node->key) < 0); + if (upper) + VERIFY(tree->cmp_func(upper, node->key) > 0); pr_node* l = node->llink; pr_node* r = node->rlink; - if (!node_verify(tree, node, l) || - !node_verify(tree, node, r)) + *count += 1; + if (!node_verify(tree, node, l, lower, node->key, count) || + !node_verify(tree, node, r, node->key, upper, count)) return false; unsigned lweight = WEIGHT(l); unsigned rweight = WEIGHT(r); @@ -485,7 +500,11 @@ pr_tree_verify(const pr_tree* tree) } else { VERIFY(tree->count == 0); } - return node_verify(tree, NULL, tree->root); + size_t count = 0; + if (!node_verify(tree, NULL, tree->root, NULL, NULL, &count)) + return false; + VERIFY(tree->count == count); + return true; } pr_itor* diff --git a/src/rb_tree.c b/src/rb_tree.c index a534b492..9cd7cedc 100644 --- a/src/rb_tree.c +++ b/src/rb_tree.c @@ -31,7 +31,6 @@ #include "rb_tree.h" -#include #include "dict_private.h" #include "tree_common.h" @@ -54,6 +53,26 @@ struct rb_node { #define SET_BLACK(node) (node)->color |= ((intptr_t)RB_BLACK) #define SET_PARENT(node,p) (node)->color = COLOR(node) | (intptr_t)(p) +/* rb_node is handed to the generic routines in tree_common.c as though it were + * a tree_node_base, so its first five members must sit at the same offsets. + * The third of those, the parent pointer, additionally carries the colour in + * its low bit, so ONLY this file's PARENT() macro may decode it: any + * tree_common routine that reads |parent| directly (tree_traverse, + * tree_select, tree_clear, tree_iterator_next/prev, tree_node_rot_*) would + * dereference a tagged pointer and must be reimplemented here. */ +_Static_assert(sizeof(intptr_t) == sizeof(rb_node*), + "rb_node tags a parent pointer stored in an intptr_t"); +_Static_assert(offsetof(rb_node, key) == offsetof(tree_node_base, key), + "rb_node must be layout-compatible with tree_node_base"); +_Static_assert(offsetof(rb_node, datum) == offsetof(tree_node_base, datum), + "rb_node must be layout-compatible with tree_node_base"); +_Static_assert(offsetof(rb_node, color) == offsetof(tree_node_base, parent), + "rb_node must be layout-compatible with tree_node_base"); +_Static_assert(offsetof(rb_node, llink) == offsetof(tree_node_base, llink), + "rb_node must be layout-compatible with tree_node_base"); +_Static_assert(offsetof(rb_node, rlink) == offsetof(tree_node_base, rlink), + "rb_node must be layout-compatible with tree_node_base"); + struct rb_tree { TREE_FIELDS(rb_node); }; @@ -62,43 +81,74 @@ struct rb_itor { TREE_ITERATOR_FIELDS(rb_tree, rb_node); }; +/* Vtable thunks. The vtable slot types take void*, and calling a function + * through a function-pointer type that is not compatible with the function's + * own type is undefined behaviour (C11 6.5.2.2p9), so each slot below gets a + * thunk with the exact slot signature instead of a cast. A cast would also + * hide a genuine signature mismatch. */ +static dict_itor* vt_rb_dict_itor_new(void* obj) { return rb_dict_itor_new(obj); } +static size_t vt_rb_tree_free(void* obj, dict_delete_func delete_func) { return rb_tree_free(obj, delete_func); } +static dict_insert_result vt_rb_tree_insert(void* obj, void* key) { return rb_tree_insert(obj, key); } +static dict_remove_result vt_rb_tree_remove(void* obj, const void* key) { return rb_tree_remove(obj, key); } +static size_t vt_rb_tree_clear(void* obj, dict_delete_func delete_func) { return rb_tree_clear(obj, delete_func); } +static size_t vt_rb_tree_traverse(void* obj, dict_visit_func visit, void* user_data) { return rb_tree_traverse(obj, visit, user_data); } +static bool vt_rb_tree_select(void* obj, size_t n, const void** key, void** datum) { return rb_tree_select(obj, n, key, datum); } +static bool vt_rb_tree_verify(const void* obj) { return rb_tree_verify(obj); } +static void vt_rb_itor_free(void* itor) { rb_itor_free(itor); } +static bool vt_rb_itor_valid(const void* itor) { return rb_itor_valid(itor); } +static void vt_rb_itor_invalidate(void* itor) { rb_itor_invalidate(itor); } +static bool vt_rb_itor_next(void* itor) { return rb_itor_next(itor); } +static bool vt_rb_itor_prev(void* itor) { return rb_itor_prev(itor); } +static bool vt_rb_itor_nextn(void* itor, size_t count) { return rb_itor_nextn(itor, count); } +static bool vt_rb_itor_prevn(void* itor, size_t count) { return rb_itor_prevn(itor, count); } +static bool vt_rb_itor_first(void* itor) { return rb_itor_first(itor); } +static bool vt_rb_itor_last(void* itor) { return rb_itor_last(itor); } +static const void* vt_rb_itor_key(const void* itor) { return rb_itor_key(itor); } +static void** vt_rb_itor_datum(void* itor) { return rb_itor_datum(itor); } +static bool vt_rb_itor_search(void* itor, const void* key) { return rb_itor_search(itor, key); } +static bool vt_rb_itor_search_le(void* itor, const void* key) { return rb_itor_search_le(itor, key); } +static bool vt_rb_itor_search_lt(void* itor, const void* key) { return rb_itor_search_lt(itor, key); } +static bool vt_rb_itor_search_ge(void* itor, const void* key) { return rb_itor_search_ge(itor, key); } +static bool vt_rb_itor_search_gt(void* itor, const void* key) { return rb_itor_search_gt(itor, key); } +static bool vt_rb_itor_remove(void* itor) { return rb_itor_remove(itor); } + static const dict_vtable rb_tree_vtable = { true, - (dict_inew_func) rb_dict_itor_new, - (dict_dfree_func) rb_tree_free, - (dict_insert_func) rb_tree_insert, - (dict_search_func) tree_search, - (dict_search_func) tree_search_le, - (dict_search_func) tree_search_lt, - (dict_search_func) tree_search_ge, - (dict_search_func) tree_search_gt, - (dict_remove_func) rb_tree_remove, - (dict_clear_func) rb_tree_clear, - (dict_traverse_func) rb_tree_traverse, - (dict_select_func) rb_tree_select, - (dict_count_func) tree_count, - (dict_verify_func) rb_tree_verify, + vt_rb_dict_itor_new, + vt_rb_tree_free, + vt_rb_tree_insert, + tree_search, + tree_search_le, + tree_search_lt, + tree_search_ge, + tree_search_gt, + vt_rb_tree_remove, + vt_rb_tree_clear, + vt_rb_tree_traverse, + vt_rb_tree_select, + tree_count, + vt_rb_tree_verify, }; static const itor_vtable rb_tree_itor_vtable = { - (dict_ifree_func) rb_itor_free, - (dict_valid_func) rb_itor_valid, - (dict_invalidate_func) rb_itor_invalidate, - (dict_next_func) rb_itor_next, - (dict_prev_func) rb_itor_prev, - (dict_nextn_func) rb_itor_nextn, - (dict_prevn_func) rb_itor_prevn, - (dict_first_func) rb_itor_first, - (dict_last_func) rb_itor_last, - (dict_key_func) rb_itor_key, - (dict_datum_func) rb_itor_datum, - (dict_isearch_func) rb_itor_search, - (dict_isearch_func) rb_itor_search_le, - (dict_isearch_func) rb_itor_search_lt, - (dict_isearch_func) rb_itor_search_ge, - (dict_isearch_func) rb_itor_search_gt, - (dict_iremove_func) rb_itor_remove, - (dict_icompare_func) tree_iterator_compare + vt_rb_itor_free, + vt_rb_itor_valid, + vt_rb_itor_invalidate, + vt_rb_itor_next, + vt_rb_itor_prev, + vt_rb_itor_nextn, + vt_rb_itor_prevn, + vt_rb_itor_first, + vt_rb_itor_last, + vt_rb_itor_key, + vt_rb_itor_datum, + vt_rb_itor_search, + vt_rb_itor_search_le, + vt_rb_itor_search_lt, + vt_rb_itor_search_ge, + vt_rb_itor_search_gt, + vt_rb_itor_remove, + tree_iterator_compare }; static void rot_left(rb_tree* tree, rb_node* node); @@ -158,12 +208,14 @@ rb_tree_clear(rb_tree* tree, dict_delete_func delete_func) } if (delete_func) delete_func(node->key, node->datum); rb_node* const parent = PARENT(node); - FREE(node); + /* Unlink before freeing: comparing a pointer to a freed object is UB. */ *(parent ? (parent->llink == node ? &parent->llink : &parent->rlink) : &tree->root) = NULL; + FREE(node); node = parent; } ASSERT(tree->root == NULL); tree->count = 0; + tree->rotation_count = 0; return count; } @@ -371,8 +423,9 @@ delete_fixup(rb_tree* tree, rb_node* node, rb_node* parent, bool left) } } - if (node) - SET_BLACK(node); + if (node) { + SET_BLACK(node); + } return rotations; } @@ -455,7 +508,15 @@ node_new(void* key) { rb_node* node = MALLOC(sizeof(*node)); if (node) { - ASSERT((((intptr_t)node) & 1) == 0); /* Ensure malloc returns aligned result. */ + /* The low bit of the parent pointer holds the colour, so a node must + * be at least 2-byte aligned. dict_malloc_func is user-replaceable, + * and an ASSERT would vanish under NDEBUG and let a misaligned + * allocator corrupt every parent pointer silently. */ + if ((((uintptr_t) node) & 1) != 0) { + fprintf(stderr, "%s:%d (%s) dict_malloc_func returned an odd " + "pointer: %p\n", __FILE__, __LINE__, __func__, (void*) node); + abort(); + } node->key = key; node->datum = NULL; node->color = RB_RED; /* Also initializes parent to NULL */ @@ -503,7 +564,8 @@ node_prev(rb_node* node) static bool node_verify(const rb_tree* tree, const rb_node* parent, const rb_node* node, - unsigned black_node_count, unsigned leaf_black_node_count) + unsigned black_node_count, unsigned leaf_black_node_count, + const void* lower, const void* upper, size_t* count) { if (parent == NULL) { VERIFY(tree->root == node); @@ -511,34 +573,36 @@ node_verify(const rb_tree* tree, const rb_node* parent, const rb_node* node, } else { VERIFY(parent->llink == node || parent->rlink == node); } - if (node) { - VERIFY(PARENT(node) == parent); - if (parent) { - if (parent->llink == node) { - VERIFY(tree->cmp_func(parent->key, node->key) > 0); - } else { - ASSERT(parent->rlink == node); - VERIFY(tree->cmp_func(parent->key, node->key) < 0); - } - } - if (COLOR(node) == RB_RED) { - /* Verify that every child of a red node is black. */ - if (node->llink) - VERIFY(COLOR(node->llink) == RB_BLACK); - if (node->rlink) - VERIFY(COLOR(node->rlink) == RB_BLACK); - } else { - black_node_count++; - } - if (!node->llink && !node->rlink) { - /* Verify that each path to a leaf contains the same number of black nodes. */ - VERIFY(black_node_count == leaf_black_node_count); - } - bool l = node_verify(tree, node, node->llink, black_node_count, leaf_black_node_count); - bool r = node_verify(tree, node, node->rlink, black_node_count, leaf_black_node_count); - return l && r; + if (!node) { + /* NIL is the leaf for black-height purposes, so the comparison + * belongs here. Checking only at nodes with no children left every + * path that terminates at the absent child of a *one-child* node + * unverified -- which is most of them. */ + VERIFY(black_node_count == leaf_black_node_count); + return true; } - return true; + VERIFY(PARENT(node) == parent); + /* Global BST ordering. Comparing a node only against its immediate parent + * would accept an entire subtree placed on the wrong side. */ + if (lower) + VERIFY(tree->cmp_func(lower, node->key) < 0); + if (upper) + VERIFY(tree->cmp_func(upper, node->key) > 0); + if (COLOR(node) == RB_RED) { + /* Verify that every child of a red node is black. */ + if (node->llink) + VERIFY(COLOR(node->llink) == RB_BLACK); + if (node->rlink) + VERIFY(COLOR(node->rlink) == RB_BLACK); + } else { + black_node_count++; + } + *count += 1; + bool l = node_verify(tree, node, node->llink, black_node_count, + leaf_black_node_count, lower, node->key, count); + bool r = node_verify(tree, node, node->rlink, black_node_count, + leaf_black_node_count, node->key, upper, count); + return l && r; } bool @@ -555,7 +619,12 @@ rb_tree_verify(const rb_tree* tree) } else { VERIFY(tree->count == 0); } - return node_verify(tree, NULL, tree->root, 0, leaf_black_node_count); + size_t count = 0; + if (!node_verify(tree, NULL, tree->root, 0, leaf_black_node_count, + NULL, NULL, &count)) + return false; + VERIFY(tree->count == count); + return true; } rb_itor* @@ -642,4 +711,4 @@ rb_itor_remove(rb_itor* it) remove_node(it->tree, it->node); it->node = NULL; return true; -} \ No newline at end of file +} diff --git a/src/skiplist.c b/src/skiplist.c index 17f8d2c2..f5ecf76f 100644 --- a/src/skiplist.c +++ b/src/skiplist.c @@ -44,6 +44,9 @@ struct skip_node { skip_node* link[]; }; +/* A node can be given at most max_link - 1 links, so max_link must be at + * least 2 for the list to be able to link anything at all. */ +#define MIN_LINK 2 #define MAX_LINK 32 struct skiplist { @@ -59,43 +62,81 @@ struct skiplist_itor { skip_node* node; }; +/* Vtable thunks. The vtable slot types take void*, and calling a function + * through a function-pointer type that is not compatible with the function's + * own type is undefined behaviour (C11 6.5.2.2p9), so each slot below gets a + * thunk with the exact slot signature instead of a cast. A cast would also + * hide a genuine signature mismatch. */ +static dict_itor* vt_skiplist_dict_itor_new(void* obj) { return skiplist_dict_itor_new(obj); } +static size_t vt_skiplist_free(void* obj, dict_delete_func delete_func) { return skiplist_free(obj, delete_func); } +static dict_insert_result vt_skiplist_insert(void* obj, void* key) { return skiplist_insert(obj, key); } +static void** vt_skiplist_search(void* obj, const void* key) { return skiplist_search(obj, key); } +static void** vt_skiplist_search_le(void* obj, const void* key) { return skiplist_search_le(obj, key); } +static void** vt_skiplist_search_lt(void* obj, const void* key) { return skiplist_search_lt(obj, key); } +static void** vt_skiplist_search_ge(void* obj, const void* key) { return skiplist_search_ge(obj, key); } +static void** vt_skiplist_search_gt(void* obj, const void* key) { return skiplist_search_gt(obj, key); } +static dict_remove_result vt_skiplist_remove(void* obj, const void* key) { return skiplist_remove(obj, key); } +static size_t vt_skiplist_clear(void* obj, dict_delete_func delete_func) { return skiplist_clear(obj, delete_func); } +static size_t vt_skiplist_traverse(void* obj, dict_visit_func visit, void* user_data) { return skiplist_traverse(obj, visit, user_data); } +static bool vt_skiplist_select(void* obj, size_t n, const void** key, void** datum) { return skiplist_select(obj, n, key, datum); } +static size_t vt_skiplist_count(const void* obj) { return skiplist_count(obj); } +static bool vt_skiplist_verify(const void* obj) { return skiplist_verify(obj); } +static void vt_skiplist_itor_free(void* itor) { skiplist_itor_free(itor); } +static bool vt_skiplist_itor_valid(const void* itor) { return skiplist_itor_valid(itor); } +static void vt_skiplist_itor_invalidate(void* itor) { skiplist_itor_invalidate(itor); } +static bool vt_skiplist_itor_next(void* itor) { return skiplist_itor_next(itor); } +static bool vt_skiplist_itor_prev(void* itor) { return skiplist_itor_prev(itor); } +static bool vt_skiplist_itor_nextn(void* itor, size_t count) { return skiplist_itor_nextn(itor, count); } +static bool vt_skiplist_itor_prevn(void* itor, size_t count) { return skiplist_itor_prevn(itor, count); } +static bool vt_skiplist_itor_first(void* itor) { return skiplist_itor_first(itor); } +static bool vt_skiplist_itor_last(void* itor) { return skiplist_itor_last(itor); } +static const void* vt_skiplist_itor_key(const void* itor) { return skiplist_itor_key(itor); } +static void** vt_skiplist_itor_datum(void* itor) { return skiplist_itor_datum(itor); } +static bool vt_skiplist_itor_search(void* itor, const void* key) { return skiplist_itor_search(itor, key); } +static bool vt_skiplist_itor_search_le(void* itor, const void* key) { return skiplist_itor_search_le(itor, key); } +static bool vt_skiplist_itor_search_lt(void* itor, const void* key) { return skiplist_itor_search_lt(itor, key); } +static bool vt_skiplist_itor_search_ge(void* itor, const void* key) { return skiplist_itor_search_ge(itor, key); } +static bool vt_skiplist_itor_search_gt(void* itor, const void* key) { return skiplist_itor_search_gt(itor, key); } +static bool vt_skiplist_itor_remove(void* itor) { return skiplist_itor_remove(itor); } +static int vt_skiplist_itor_compare(const void* itor1, const void* itor2) { return skiplist_itor_compare(itor1, itor2); } + static const dict_vtable skiplist_vtable = { true, - (dict_inew_func) skiplist_dict_itor_new, - (dict_dfree_func) skiplist_free, - (dict_insert_func) skiplist_insert, - (dict_search_func) skiplist_search, - (dict_search_func) skiplist_search_le, - (dict_search_func) skiplist_search_lt, - (dict_search_func) skiplist_search_ge, - (dict_search_func) skiplist_search_gt, - (dict_remove_func) skiplist_remove, - (dict_clear_func) skiplist_clear, - (dict_traverse_func) skiplist_traverse, - (dict_select_func) NULL, - (dict_count_func) skiplist_count, - (dict_verify_func) skiplist_verify, + vt_skiplist_dict_itor_new, + vt_skiplist_free, + vt_skiplist_insert, + vt_skiplist_search, + vt_skiplist_search_le, + vt_skiplist_search_lt, + vt_skiplist_search_ge, + vt_skiplist_search_gt, + vt_skiplist_remove, + vt_skiplist_clear, + vt_skiplist_traverse, + vt_skiplist_select, + vt_skiplist_count, + vt_skiplist_verify, }; static const itor_vtable skiplist_itor_vtable = { - (dict_ifree_func) skiplist_itor_free, - (dict_valid_func) skiplist_itor_valid, - (dict_invalidate_func) skiplist_itor_invalidate, - (dict_next_func) skiplist_itor_next, - (dict_prev_func) skiplist_itor_prev, - (dict_nextn_func) skiplist_itor_nextn, - (dict_prevn_func) skiplist_itor_prevn, - (dict_first_func) skiplist_itor_first, - (dict_last_func) skiplist_itor_last, - (dict_key_func) skiplist_itor_key, - (dict_datum_func) skiplist_itor_datum, - (dict_isearch_func) skiplist_itor_search, - (dict_isearch_func) skiplist_itor_search_le, - (dict_isearch_func) skiplist_itor_search_lt, - (dict_isearch_func) skiplist_itor_search_ge, - (dict_isearch_func) skiplist_itor_search_gt, - (dict_iremove_func) skiplist_itor_remove, - (dict_icompare_func) skiplist_itor_compare, + vt_skiplist_itor_free, + vt_skiplist_itor_valid, + vt_skiplist_itor_invalidate, + vt_skiplist_itor_next, + vt_skiplist_itor_prev, + vt_skiplist_itor_nextn, + vt_skiplist_itor_prevn, + vt_skiplist_itor_first, + vt_skiplist_itor_last, + vt_skiplist_itor_key, + vt_skiplist_itor_datum, + vt_skiplist_itor_search, + vt_skiplist_itor_search_le, + vt_skiplist_itor_search_lt, + vt_skiplist_itor_search_ge, + vt_skiplist_itor_search_gt, + vt_skiplist_itor_remove, + vt_skiplist_itor_compare, }; static inline skip_node* node_new(void* key, unsigned link_count); @@ -111,9 +152,15 @@ skiplist* skiplist_new(dict_compare_func cmp_func, unsigned max_link) { ASSERT(cmp_func != NULL); - ASSERT(max_link > 0); - if (max_link > MAX_LINK) + /* Clamp both ends rather than trusting the caller, consistently with how + * the upper bound has always been handled. The lower bound matters: + * rand_link_count() can produce at most max_link - 1 links, so a max_link + * below MIN_LINK would yield zero-link nodes that node_insert() silently + * fails to link into the list -- losing every element and leaking it. */ + if (max_link < MIN_LINK) + max_link = MIN_LINK; + else if (max_link > MAX_LINK) max_link = MAX_LINK; skiplist* list = MALLOC(sizeof(*list)); @@ -441,6 +488,28 @@ skiplist_count(const skiplist* list) return list->count; } +bool +skiplist_select(skiplist* list, size_t n, const void** key, void** datum) +{ + if (n >= list->count) { + if (key) + *key = NULL; + if (datum) + *datum = NULL; + return false; + } + /* No span counts are maintained, so this walks the bottom level: O(n) + * rather than the O(lg n) the trees manage. */ + skip_node* node = list->head->link[0]; + while (n--) + node = node->link[0]; + if (key) + *key = node->key; + if (datum) + *datum = node->datum; + return true; +} + bool skiplist_verify(const skiplist* list) { @@ -457,6 +526,7 @@ skiplist_verify(const skiplist* list) VERIFY(list->head->link[i] == NULL); } unsigned observed_top_link = 0; + size_t observed_count = 0; skip_node* prev = NULL; skip_node* node = list->head->link[0]; @@ -469,14 +539,20 @@ skiplist_verify(const skiplist* list) VERIFY(node->link_count <= list->top_link); for (unsigned k = 0; k < node->link_count; k++) { if (node->link[k]) { - VERIFY(node->link[k]->link_count >= k); + /* To occupy level k a node needs links 0..k, i.e. more than k + * of them. */ + VERIFY(node->link[k]->link_count > k); + /* Links must run strictly forward in key order. */ + VERIFY(list->cmp_func(node->key, node->link[k]->key) < 0); } } + ++observed_count; prev = node; node = node->link[0]; } VERIFY(list->top_link == observed_top_link); + VERIFY(list->count == observed_count); return true; } @@ -656,9 +732,8 @@ skiplist_itor_remove(skiplist_itor* itor) return false; /* XXX make this smarter */ dict_remove_result result = skiplist_remove(itor->list, itor->node->key); - ASSERT(result.removed); itor->node = NULL; - return true; + return result.removed; } static inline skip_node* @@ -678,9 +753,16 @@ node_new(void* key, unsigned link_count) return node; } +/* Returns a link count in [1, max_link - 1], geometrically distributed with + * ratio 1/4 (two coin flips per level). */ static inline unsigned rand_link_count(skiplist* list) { - unsigned count = (unsigned) __builtin_ctz(dict_rand()) / 2 + 1; - return (count >= list->max_link) ? list->max_link - 1 : count; + /* dict_ctz() requires a nonzero argument and random() does return 0, so + * fold zero onto the largest trailing-zero count it could represent. */ + const unsigned r = dict_rand(); + const unsigned tz = r ? dict_ctz(r) : DICT_RAND_BITS; + const unsigned count = tz / 2 + 1; + ASSERT(list->max_link >= MIN_LINK); + return MIN(count, list->max_link - 1); } \ No newline at end of file diff --git a/src/sp_tree.c b/src/sp_tree.c index 6278a938..e7966e02 100644 --- a/src/sp_tree.c +++ b/src/sp_tree.c @@ -31,7 +31,7 @@ * A single operation on a splay tree has a worst-case time complexity of O(N), * but a series of M operations have a time complexity of O(M lg N), and thus * the amortized time complexity of an operation is O(lg N). More specifically, - * a series of M operations on a tree with N nodes will runs in O((N+M)lg(N+M)) + * a series of M operations on a tree with N nodes will run in O((N+M)lg(N+M)) * time. Splay trees work by "splaying" a node up the tree using a series of * rotations until it is the root each time it is accessed. They are much * simpler to code than most balanced trees, because there is no strict @@ -63,43 +63,60 @@ struct sp_itor { TREE_ITERATOR_FIELDS(sp_tree, sp_node); }; +/* Vtable thunks. The vtable slot types take void*, and calling a function + * through a function-pointer type that is not compatible with the function's + * own type is undefined behaviour (C11 6.5.2.2p9), so each slot below gets a + * thunk with the exact slot signature instead of a cast. A cast would also + * hide a genuine signature mismatch. */ +static dict_itor* vt_sp_dict_itor_new(void* obj) { return sp_dict_itor_new(obj); } +static dict_insert_result vt_sp_tree_insert(void* obj, void* key) { return sp_tree_insert(obj, key); } +static void** vt_sp_tree_search(void* obj, const void* key) { return sp_tree_search(obj, key); } +static void** vt_sp_tree_search_le(void* obj, const void* key) { return sp_tree_search_le(obj, key); } +static void** vt_sp_tree_search_lt(void* obj, const void* key) { return sp_tree_search_lt(obj, key); } +static void** vt_sp_tree_search_ge(void* obj, const void* key) { return sp_tree_search_ge(obj, key); } +static void** vt_sp_tree_search_gt(void* obj, const void* key) { return sp_tree_search_gt(obj, key); } +static dict_remove_result vt_sp_tree_remove(void* obj, const void* key) { return sp_tree_remove(obj, key); } +static bool vt_sp_tree_verify(const void* obj) { return sp_tree_verify(obj); } +static bool vt_sp_itor_search(void* itor, const void* key) { return sp_itor_search(itor, key); } +static bool vt_sp_itor_remove(void* itor) { return sp_itor_remove(itor); } + static const dict_vtable sp_tree_vtable = { true, - (dict_inew_func) sp_dict_itor_new, - (dict_dfree_func) tree_free, - (dict_insert_func) sp_tree_insert, - (dict_search_func) sp_tree_search, - (dict_search_func) sp_tree_search_le, - (dict_search_func) sp_tree_search_lt, - (dict_search_func) sp_tree_search_ge, - (dict_search_func) sp_tree_search_gt, - (dict_remove_func) sp_tree_remove, - (dict_clear_func) tree_clear, - (dict_traverse_func) tree_traverse, - (dict_select_func) tree_select, - (dict_count_func) tree_count, - (dict_verify_func) sp_tree_verify, + vt_sp_dict_itor_new, + tree_free, + vt_sp_tree_insert, + vt_sp_tree_search, + vt_sp_tree_search_le, + vt_sp_tree_search_lt, + vt_sp_tree_search_ge, + vt_sp_tree_search_gt, + vt_sp_tree_remove, + tree_clear, + tree_traverse, + tree_select, + tree_count, + vt_sp_tree_verify, }; static const itor_vtable sp_tree_itor_vtable = { - (dict_ifree_func) tree_iterator_free, - (dict_valid_func) tree_iterator_valid, - (dict_invalidate_func) tree_iterator_invalidate, - (dict_next_func) tree_iterator_next, - (dict_prev_func) tree_iterator_prev, - (dict_nextn_func) tree_iterator_nextn, - (dict_prevn_func) tree_iterator_prevn, - (dict_first_func) tree_iterator_first, - (dict_last_func) tree_iterator_last, - (dict_key_func) tree_iterator_key, - (dict_datum_func) tree_iterator_datum, - (dict_isearch_func) sp_itor_search, - (dict_isearch_func) tree_iterator_search_le, - (dict_isearch_func) tree_iterator_search_lt, - (dict_isearch_func) tree_iterator_search_ge, - (dict_isearch_func) tree_iterator_search_gt, - (dict_iremove_func) sp_itor_remove, - (dict_icompare_func) tree_iterator_compare + tree_iterator_free, + tree_iterator_valid, + tree_iterator_invalidate, + tree_iterator_next, + tree_iterator_prev, + tree_iterator_nextn, + tree_iterator_prevn, + tree_iterator_first, + tree_iterator_last, + tree_iterator_key, + tree_iterator_datum, + vt_sp_itor_search, + tree_iterator_search_le, + tree_iterator_search_lt, + tree_iterator_search_ge, + tree_iterator_search_gt, + vt_sp_itor_remove, + tree_iterator_compare }; static sp_node* node_new(void* key); @@ -154,7 +171,6 @@ splay(sp_tree* t, sp_node* n) if ((p->llink = n->rlink) != NULL) p->llink->parent = p; n->rlink = p; - ++rotations; } else { if ((p->rlink = n->llink) != NULL) p->rlink->parent = p; @@ -252,8 +268,13 @@ sp_tree_insert(sp_tree* tree, void* key) parent = node; node = node->llink; } else if (cmp > 0) { parent = node; node = node->rlink; - } else + } else { + /* Splay on a duplicate too: every other accessor restructures, and + * skipping it here breaks the amortized O(lg n) bound for repeated + * inserts of an existing key. */ + splay(tree, node); return (dict_insert_result) { &node->datum, false }; + } } if (!(node = node_new(key))) @@ -263,7 +284,7 @@ sp_tree_insert(sp_tree* tree, void* key) ASSERT(tree->count == 0); ASSERT(tree->root == NULL); tree->root = node; - tree->count = 1; + tree->count++; } else { if (cmp < 0) parent->llink = node; @@ -361,11 +382,12 @@ remove_node(sp_tree* tree, sp_node* node) sp_node* const temp = out->llink ? out->llink : out->rlink; sp_node* const parent = out->parent; + /* Splice before freeing: comparing a pointer to a freed object is UB. */ + *(parent ? (parent->llink == out ? &parent->llink : &parent->rlink) : &tree->root) = temp; FREE(out); if (temp) temp->parent = parent; - *(parent ? (parent->llink == out ? &parent->llink : &parent->rlink) : &tree->root) = temp; if (parent) splay(tree, parent); tree->count--; @@ -403,40 +425,14 @@ node_new(void* key) return node; } -static bool -node_verify(const sp_tree* tree, const sp_node* parent, const sp_node* node) -{ - if (!parent) { - VERIFY(tree->root == node); - } else { - VERIFY(parent->llink == node || parent->rlink == node); - } - if (node) { - VERIFY(node->parent == parent); - if (parent) { - if (parent->llink == node) { - VERIFY(tree->cmp_func(parent->key, node->key) > 0); - } else { - ASSERT(parent->rlink == node); - VERIFY(tree->cmp_func(parent->key, node->key) < 0); - } - } - if (!node_verify(tree, node, node->llink) || - !node_verify(tree, node, node->rlink)) - return false; - } - return true; -} - bool sp_tree_verify(const sp_tree* tree) { - if (tree->root) { - VERIFY(tree->count > 0); - } else { - VERIFY(tree->count == 0); - } - return node_verify(tree, NULL, tree->root); + /* A splay tree has no shape invariant of its own beyond being a valid + * search tree, so the shared iterative check is the whole job. It must be + * iterative: ascending inserts make a chain as deep as the tree is large, + * and recursion would overflow the stack on exactly that case. */ + return tree_verify_common(tree); } sp_itor* diff --git a/src/tr_tree.c b/src/tr_tree.c index 947667a3..95f0b033 100644 --- a/src/tr_tree.c +++ b/src/tr_tree.c @@ -28,13 +28,15 @@ /* * cf. [Aragon and Seidel, 1996], [Knuth 1998] * - * A treap is a randomized data structure in which each node of tree has an + * A treap is a randomized data structure in which each node of the tree has an * associated key and priority. The priority is chosen at random when the node * is inserted into the tree. Each node is inserted so that the lexicographic - * order of the keys is preserved, and the priority of any node is less than - * the priority of either of its child nodes; in this way the treap is a - * combination of a tree and a min-heap. In this implementation, this is - * accomplished by first inserting the node according to lexigraphical order of + * order of the keys is preserved, and the priority of any node is greater + * than or equal to the priority of either of its child nodes; in this way the + * treap is a combination of a tree and a max-heap. Note the direction: a + * dict_prio_func should return a LARGER value for a key that ought to sit + * closer to the root. In this implementation, this is + * accomplished by first inserting the node according to lexicographical order of * keys as in a normal binary tree, and then, if needed, sifting the node * upwards using a series of rotations until the heap property of the tree is * restored. @@ -42,7 +44,6 @@ #include "tr_tree.h" -#include #include "dict_private.h" #include "tree_common.h" @@ -61,43 +62,54 @@ struct tr_itor { TREE_ITERATOR_FIELDS(tr_tree, tr_node); }; +/* Vtable thunks. The vtable slot types take void*, and calling a function + * through a function-pointer type that is not compatible with the function's + * own type is undefined behaviour (C11 6.5.2.2p9), so each slot below gets a + * thunk with the exact slot signature instead of a cast. A cast would also + * hide a genuine signature mismatch. */ +static dict_itor* vt_tr_dict_itor_new(void* obj) { return tr_dict_itor_new(obj); } +static dict_insert_result vt_tr_tree_insert(void* obj, void* key) { return tr_tree_insert(obj, key); } +static dict_remove_result vt_tr_tree_remove(void* obj, const void* key) { return tr_tree_remove(obj, key); } +static bool vt_tr_tree_verify(const void* obj) { return tr_tree_verify(obj); } +static bool vt_tr_itor_remove(void* itor) { return tr_itor_remove(itor); } + static const dict_vtable tr_tree_vtable = { true, - (dict_inew_func) tr_dict_itor_new, - (dict_dfree_func) tree_free, - (dict_insert_func) tr_tree_insert, - (dict_search_func) tree_search, - (dict_search_func) tree_search_le, - (dict_search_func) tree_search_lt, - (dict_search_func) tree_search_ge, - (dict_search_func) tree_search_gt, - (dict_remove_func) tr_tree_remove, - (dict_clear_func) tree_clear, - (dict_traverse_func) tree_traverse, - (dict_select_func) tree_select, - (dict_count_func) tree_count, - (dict_verify_func) tr_tree_verify, + vt_tr_dict_itor_new, + tree_free, + vt_tr_tree_insert, + tree_search, + tree_search_le, + tree_search_lt, + tree_search_ge, + tree_search_gt, + vt_tr_tree_remove, + tree_clear, + tree_traverse, + tree_select, + tree_count, + vt_tr_tree_verify, }; static const itor_vtable tr_tree_itor_vtable = { - (dict_ifree_func) tree_iterator_free, - (dict_valid_func) tree_iterator_valid, - (dict_invalidate_func) tree_iterator_invalidate, - (dict_next_func) tree_iterator_next, - (dict_prev_func) tree_iterator_prev, - (dict_nextn_func) tree_iterator_nextn, - (dict_prevn_func) tree_iterator_prevn, - (dict_first_func) tree_iterator_first, - (dict_last_func) tree_iterator_last, - (dict_key_func) tree_iterator_key, - (dict_datum_func) tree_iterator_datum, - (dict_isearch_func) tree_iterator_search, - (dict_isearch_func) tree_iterator_search_le, - (dict_isearch_func) tree_iterator_search_lt, - (dict_isearch_func) tree_iterator_search_ge, - (dict_isearch_func) tree_iterator_search_gt, - (dict_iremove_func) tr_itor_remove, - (dict_icompare_func) tree_iterator_compare, + tree_iterator_free, + tree_iterator_valid, + tree_iterator_invalidate, + tree_iterator_next, + tree_iterator_prev, + tree_iterator_nextn, + tree_iterator_prevn, + tree_iterator_first, + tree_iterator_last, + tree_iterator_key, + tree_iterator_datum, + tree_iterator_search, + tree_iterator_search_le, + tree_iterator_search_lt, + tree_iterator_search_ge, + tree_iterator_search_gt, + vt_tr_itor_remove, + tree_iterator_compare, }; static tr_node* node_new(void* key); @@ -237,47 +249,46 @@ node_new(void* key) node->parent = NULL; node->llink = NULL; node->rlink = NULL; + /* Overwritten by the caller, but leaving it uninitialized would make a + * future second caller inherit heap garbage as a priority. */ + node->prio = 0; } return node; } -static bool -node_verify(const tr_tree* tree, const tr_node* parent, const tr_node* node) +bool +tr_tree_verify(const tr_tree* tree) { - if (!parent) { - VERIFY(tree->root == node); - } else { - VERIFY(parent->llink == node || parent->rlink == node); - } - if (node) { - VERIFY(node->parent == parent); - if (parent) { - VERIFY(node->prio <= parent->prio); - if (parent->llink == node) { - VERIFY(tree->cmp_func(parent->key, node->key) > 0); - } else { - ASSERT(parent->rlink == node); - VERIFY(tree->cmp_func(parent->key, node->key) < 0); + /* Search-tree, parent-link and count invariants. Iterative, because a + * treap given a poor priority function degenerates into a chain as deep + * as it has nodes. */ + if (!tree_verify_common(tree)) + return false; + + /* Max-heap property on priorities. Now that the links have been verified + * an in-order walk is safe, and it needs no stack of its own. */ + const tr_node* node = tree->root; + while (node && node->llink) + node = node->llink; + while (node) { + if (node->parent) + VERIFY(node->prio <= node->parent->prio); + if (node->rlink) { + node = node->rlink; + while (node->llink) + node = node->llink; + } else { + const tr_node* parent = node->parent; + while (parent && parent->rlink == node) { + node = parent; + parent = parent->parent; } + node = parent; } - if (!node_verify(tree, node, node->llink) || - !node_verify(tree, node, node->rlink)) - return false; } return true; } -bool -tr_tree_verify(const tr_tree* tree) -{ - if (tree->root) { - VERIFY(tree->count > 0); - } else { - VERIFY(tree->count == 0); - } - return node_verify(tree, NULL, tree->root); -} - tr_itor* tr_itor_new(tr_tree* tree) { diff --git a/src/tree_common.c b/src/tree_common.c index cf6b7389..b955ae35 100644 --- a/src/tree_common.c +++ b/src/tree_common.c @@ -271,8 +271,10 @@ tree_select(void *Tree, size_t n, const void **key, void **datum) { tree* t = Tree; if (n >= t->count) { - *key = NULL; - *datum = NULL; + if (key) + *key = NULL; + if (datum) + *datum = NULL; return false; } tree_node* node; @@ -286,8 +288,10 @@ tree_select(void *Tree, size_t n, const void **key, void **datum) while (n--) node = tree_node_next(node); } - *key = node->key; - *datum = node->datum; + if (key) + *key = node->key; + if (datum) + *datum = node->datum; return true; } @@ -297,6 +301,96 @@ tree_count(const void* Tree) return ((const tree*)Tree)->count; } +/* Verifies the invariants every tree shares: root/parent link consistency, + * global key ordering, and that |count| matches the number of nodes reachable. + * + * This is iterative for the same reason the path-length statistics are: a + * degenerate sp_tree or tr_tree is as deep as it has nodes, so a recursive + * checker would overflow the stack on the very trees most likely to be + * malformed. It may only be used by trees with an untagged parent pointer -- + * hb_tree and rb_tree pack bits into that field and must verify themselves. */ +bool +tree_verify_common(const void* Tree) +{ + const tree* t = Tree; + + VERIFY(t->root == NULL || t->root->parent == NULL); + if (t->root) { + VERIFY(t->count > 0); + } else { + VERIFY(t->count == 0); + } + + /* (node, lower bound, upper bound) triples still to visit. A NULL bound + * means unbounded on that side. */ + struct frame { const tree_node* node; const void* lower; const void* upper; }; + struct frame* stack = NULL; + size_t size = 0, capacity = 0, count = 0; + bool ok = true, oom = false; + +#define PUSH(n, lo, hi) \ + do { \ + if (size == capacity) { \ + const size_t cap = capacity ? capacity * 2 : 32; \ + struct frame* items = MALLOC(cap * sizeof(*items)); \ + if (!items) { oom = true; break; } \ + if (stack) { \ + memcpy(items, stack, size * sizeof(*items)); \ + FREE(stack); \ + } \ + stack = items; \ + capacity = cap; \ + } \ + stack[size].node = (n); \ + stack[size].lower = (lo); \ + stack[size].upper = (hi); \ + ++size; \ + } while (0) + + if (t->root) + PUSH(t->root, NULL, NULL); + + while (ok && !oom && size) { + const struct frame top = stack[--size]; + const tree_node* const node = top.node; + + /* Global key ordering. Comparing a node only against its immediate + * parent would accept an entire subtree placed on the wrong side. */ + if (top.lower && t->cmp_func(top.lower, node->key) >= 0) { + ok = false; + break; + } + if (top.upper && t->cmp_func(top.upper, node->key) <= 0) { + ok = false; + break; + } + if (node->llink && node->llink->parent != node) { + ok = false; + break; + } + if (node->rlink && node->rlink->parent != node) { + ok = false; + break; + } + ++count; + if (node->llink) + PUSH(node->llink, top.lower, node->key); + if (!oom && node->rlink) + PUSH(node->rlink, node->key, top.upper); + } +#undef PUSH + if (stack) + FREE(stack); + + if (oom) { + fprintf(stderr, "%s:%d (%s) out of memory\n", __FILE__, __LINE__, __func__); + return false; + } + VERIFY(ok); + VERIFY(t->count == count); + return true; +} + size_t tree_clear(void* Tree, dict_delete_func delete_func) { @@ -311,8 +405,9 @@ tree_clear(void* Tree, dict_delete_func delete_func) } if (delete_func) delete_func(node->key, node->datum); tree_node* const parent = node->parent; - FREE(node); + /* Unlink before freeing: comparing a pointer to a freed object is UB. */ *(parent ? (parent->llink == node ? &parent->llink : &parent->rlink) : &t->root) = NULL; + FREE(node); node = parent; } ASSERT(t->root == NULL); @@ -328,49 +423,119 @@ tree_free(void* Tree, dict_delete_func delete_func) return count; } -static size_t -node_min_path_length(const tree_node* node) -{ - size_t l = node->llink ? node_min_path_length(node->llink) : 0; - size_t r = node->rlink ? node_min_path_length(node->rlink) : 0; - return 1 + MIN(l, r); -} +/* The three path-length statistics below share one iterative traversal. + * + * They must not recurse: a degenerate sp_tree or tr_tree (e.g. a treap given a + * constant priority function) is as deep as it has nodes, so recursion would + * overflow the stack on a few hundred thousand elements. They must also not + * walk parent pointers, because hb_tree and rb_tree pack bits into that field + * and only their own PARENT() macro may decode it -- so the traversal keeps an + * explicit, growable stack of (node, depth) pairs instead. + * + * Depths are 1-based, i.e. the root is at depth 1. */ +typedef struct { + const tree_node* node; + size_t depth; +} node_depth; -size_t -tree_min_path_length(const void* Tree) -{ - const tree* t = Tree; - return t->root ? node_min_path_length(t->root) : 0; +typedef struct { + node_depth* items; + size_t size; + size_t capacity; +} node_stack; + +/* Returns false only if memory could not be obtained. */ +static bool +node_stack_push(node_stack* stack, const tree_node* node, size_t depth) +{ + if (stack->size == stack->capacity) { + const size_t capacity = stack->capacity ? stack->capacity * 2 : 32; + node_depth* items = MALLOC(capacity * sizeof(*items)); + if (!items) + return false; + if (stack->items) { + memcpy(items, stack->items, stack->size * sizeof(*items)); + FREE(stack->items); + } + stack->items = items; + stack->capacity = capacity; + } + stack->items[stack->size].node = node; + stack->items[stack->size].depth = depth; + ++stack->size; + return true; } -static size_t -node_max_path_length(const tree_node* node) -{ - size_t l = node->llink ? node_max_path_length(node->llink) : 0; - size_t r = node->rlink ? node_max_path_length(node->rlink) : 0; - return 1 + MAX(l, r); +/* Computes, over all nodes of |t|: + * |min_path| the shallowest depth at which a child link is absent, + * |max_path| the greatest node depth, + * |total_path| the sum of all node depths. + * Any output pointer may be NULL. Returns false if the traversal ran out of + * memory, in which case the outputs are not written. */ +static bool +tree_path_lengths(const tree* t, size_t* min_path, size_t* max_path, + size_t* total_path) +{ + size_t min_len = 0, max_len = 0, total_len = 0; + node_stack stack = { NULL, 0, 0 }; + bool ok = true; + + if (t->root && !node_stack_push(&stack, t->root, 1)) + ok = false; + + while (ok && stack.size) { + const node_depth top = stack.items[--stack.size]; + const tree_node* const node = top.node; + const size_t depth = top.depth; + + if (depth > max_len) + max_len = depth; + total_len += depth; + if (!node->llink || !node->rlink) { + if (!min_len || depth < min_len) + min_len = depth; + } + if (node->llink && !node_stack_push(&stack, node->llink, depth + 1)) + ok = false; + if (ok && node->rlink && !node_stack_push(&stack, node->rlink, depth + 1)) + ok = false; + } + if (stack.items) + FREE(stack.items); + + if (!ok) + return false; + if (min_path) + *min_path = min_len; + if (max_path) + *max_path = max_len; + if (total_path) + *total_path = total_len; + return true; } size_t -tree_max_path_length(const void* Tree) +tree_min_path_length(const void* Tree) { - const tree* t = Tree; - return t->root ? node_max_path_length(t->root) : 0; + size_t len = 0; + tree_path_lengths(Tree, &len, NULL, NULL); + return len; } -static size_t -node_path_length(const tree_node* node, size_t level) +size_t +tree_max_path_length(const void* Tree) { - return level - + (node->llink ? node_path_length(node->llink, level + 1) : 0) - + (node->rlink ? node_path_length(node->rlink, level + 1) : 0); + size_t len = 0; + tree_path_lengths(Tree, NULL, &len, NULL); + return len; } size_t tree_total_path_length(const void* Tree) { - const tree* t = Tree; - return t->root ? node_path_length(t->root, 1) : 0; + size_t len = 0; + tree_path_lengths(Tree, NULL, NULL, &len); + return len; } bool diff --git a/src/tree_common.h b/src/tree_common.h index eec3ca7e..a1b08a3b 100644 --- a/src/tree_common.h +++ b/src/tree_common.h @@ -37,6 +37,9 @@ node_type* llink; \ node_type* rlink +/* The layout that every tree's node type must be compatible with, since the + * generic routines below reinterpret nodes as this type. hb_tree and rb_tree + * static_assert their field offsets against it. */ typedef struct tree_node_base { TREE_NODE_FIELDS(struct tree_node_base); } tree_node_base; @@ -67,29 +70,29 @@ void* tree_node_prev(void *node); /* Return the successor of |node|, or NULL if |node| has no successor. * |node| must not be NULL. */ void* tree_node_next(void *node); -/* Return the left child of |node|, or |node| if it has no right child. - * |node| must not be NULL. */ +/* Return the leftmost descendant of |node|, or |node| itself if it has no left + * child. Returns NULL if |node| is NULL. */ void* tree_node_min(void *node); -/* Return the rightmost child of |node|, or |node| if it has no right child. - * |node| must not be NULL. */ +/* Return the rightmost descendant of |node|, or |node| itself if it has no + * right child. Returns NULL if |node| is NULL. */ void* tree_node_max(void *node); /* Return the address of the data for the given the key, or NULL if not found. */ void** tree_search(void *tree, const void *key); -/* Return the node has the key, or NULL if not found. */ +/* Return the node that has the key, or NULL if not found. */ void* tree_search_node(void *tree, const void *key); -/* Return the data/node associated with the first key less than or +/* Return the data/node associated with the greatest key less than or * equal to the specified key, or NULL if not found. */ void** tree_search_le(void *tree, const void *key); void* tree_search_le_node(void *tree, const void *key); -/* Return the data/node associated with the first key less than the +/* Return the data/node associated with the greatest key less than the * specified key, or NULL if not found. */ void** tree_search_lt(void *tree, const void *key); void* tree_search_lt_node(void *tree, const void *key); -/* Return the data/node associated with the first key greater than or +/* Return the data/node associated with the least key greater than or * equal to the specified key, or NULL if not found. */ void** tree_search_ge(void *tree, const void *key); void* tree_search_ge_node(void *tree, const void *key); -/* Return the data/node associated with the first key greater than the +/* Return the data/node associated with the least key greater than the * specified key, or NULL if not found. */ void** tree_search_gt(void *tree, const void *key); void* tree_search_gt_node(void *tree, const void *key); @@ -99,19 +102,26 @@ void* tree_search_gt_node(void *tree, const void *key); size_t tree_traverse(void *tree, dict_visit_func visit, void* user_data); /* Put the key and datum of the |n|th element of |tree| into |key| and |datum| * and return true, or, if n is greater than or equal to the number of elements, - * return false. */ + * store NULL through each and return false. Either |key| or |datum| may be + * NULL, in which case that value is not stored. */ bool tree_select(void *tree, size_t n, const void **key, void **datum); +/* Verify the invariants shared by all trees (root/parent links, global key + * ordering, and count). Iterative, so it is safe on a degenerate tree. Must + * not be used by trees that tag their parent pointer (hb_tree, rb_tree). */ +bool tree_verify_common(const void *tree); /* Return a count of the elements in |tree|. */ size_t tree_count(const void *tree); /* Remove all elements from |tree|. */ size_t tree_clear(void *tree, dict_delete_func delete_func); /* Remove all elements from |tree| and free its memory. */ size_t tree_free(void *tree, dict_delete_func delete_func); -/* Returns the depth of the leaf with minimal depth, or 0 for an empty tree. */ +/* Returns the shallowest depth at which a child link is absent (the root being + * at depth 1), or 0 for an empty tree. */ size_t tree_min_path_length(const void *tree); -/* Returns the depth of the leaf with maximal depth, or 0 for an empty tree. */ +/* Returns the greatest depth of any node (the root being at depth 1), or 0 for + * an empty tree. */ size_t tree_max_path_length(const void *tree); -/* Returns the total path length of the tree. */ +/* Returns the sum of the depths of all nodes, or 0 for an empty tree. */ size_t tree_total_path_length(const void *tree); bool tree_iterator_valid(const void *iterator); diff --git a/src/wb_tree.c b/src/wb_tree.c index d8d2d89f..0911099a 100644 --- a/src/wb_tree.c +++ b/src/wb_tree.c @@ -31,14 +31,14 @@ #include "wb_tree.h" -#include +#include #include "dict_private.h" #include "tree_common.h" /* A tree BB[alpha] is said to be of weighted balance alpha if every node in * the tree has a balance p(n) such that alpha <= p(n) <= 1 - alpha. The * balance of a node is defined as the number of nodes in its left subtree - * divided by the number of nodes in either subtree. The weight of a node is + * divided by the number of nodes in the subtree rooted at that node. The weight of a node is * defined as the number of external nodes in its subtrees. * * Legal values for alpha are 0 <= alpha <= 1/2. BB[0] is a normal, unbalanced @@ -79,43 +79,55 @@ struct wb_itor { TREE_ITERATOR_FIELDS(wb_tree, wb_node); }; +/* Vtable thunks. The vtable slot types take void*, and calling a function + * through a function-pointer type that is not compatible with the function's + * own type is undefined behaviour (C11 6.5.2.2p9), so each slot below gets a + * thunk with the exact slot signature instead of a cast. A cast would also + * hide a genuine signature mismatch. */ +static dict_itor* vt_wb_dict_itor_new(void* obj) { return wb_dict_itor_new(obj); } +static dict_insert_result vt_wb_tree_insert(void* obj, void* key) { return wb_tree_insert(obj, key); } +static dict_remove_result vt_wb_tree_remove(void* obj, const void* key) { return wb_tree_remove(obj, key); } +static bool vt_wb_tree_select(void* obj, size_t n, const void** key, void** datum) { return wb_tree_select(obj, n, key, datum); } +static bool vt_wb_tree_verify(const void* obj) { return wb_tree_verify(obj); } +static bool vt_wb_itor_remove(void* itor) { return wb_itor_remove(itor); } + static const dict_vtable wb_tree_vtable = { true, - (dict_inew_func) wb_dict_itor_new, - (dict_dfree_func) tree_free, - (dict_insert_func) wb_tree_insert, - (dict_search_func) tree_search, - (dict_search_func) tree_search_le, - (dict_search_func) tree_search_lt, - (dict_search_func) tree_search_ge, - (dict_search_func) tree_search_gt, - (dict_remove_func) wb_tree_remove, - (dict_clear_func) tree_clear, - (dict_traverse_func) tree_traverse, - (dict_select_func) wb_tree_select, - (dict_count_func) tree_count, - (dict_verify_func) wb_tree_verify, + vt_wb_dict_itor_new, + tree_free, + vt_wb_tree_insert, + tree_search, + tree_search_le, + tree_search_lt, + tree_search_ge, + tree_search_gt, + vt_wb_tree_remove, + tree_clear, + tree_traverse, + vt_wb_tree_select, + tree_count, + vt_wb_tree_verify, }; static const itor_vtable wb_tree_itor_vtable = { - (dict_ifree_func) tree_iterator_free, - (dict_valid_func) tree_iterator_valid, - (dict_invalidate_func) tree_iterator_invalidate, - (dict_next_func) tree_iterator_next, - (dict_prev_func) tree_iterator_prev, - (dict_nextn_func) tree_iterator_nextn, - (dict_prevn_func) tree_iterator_prevn, - (dict_first_func) tree_iterator_first, - (dict_last_func) tree_iterator_last, - (dict_key_func) tree_iterator_key, - (dict_datum_func) tree_iterator_datum, - (dict_isearch_func) tree_iterator_search, - (dict_isearch_func) tree_iterator_search_le, - (dict_isearch_func) tree_iterator_search_lt, - (dict_isearch_func) tree_iterator_search_ge, - (dict_isearch_func) tree_iterator_search_gt, - (dict_iremove_func) wb_itor_remove, - (dict_icompare_func) tree_iterator_compare, + tree_iterator_free, + tree_iterator_valid, + tree_iterator_invalidate, + tree_iterator_next, + tree_iterator_prev, + tree_iterator_nextn, + tree_iterator_prevn, + tree_iterator_first, + tree_iterator_last, + tree_iterator_key, + tree_iterator_datum, + tree_iterator_search, + tree_iterator_search_le, + tree_iterator_search_lt, + tree_iterator_search_ge, + tree_iterator_search_gt, + vt_wb_itor_remove, + tree_iterator_compare, }; static wb_node* node_new(void* key); @@ -160,11 +172,11 @@ fixup(wb_tree* tree, wb_node* n) { unsigned rotations = 0; unsigned weight = WEIGHT(n->llink); - if (weight * 1000U < n->weight * 293U) { + if ((uint64_t) weight * 1000U < (uint64_t) n->weight * 293U) { wb_node* nr = n->rlink; ASSERT(nr != NULL); wb_node* nrl = nr->llink; - if (WEIGHT(nrl) * 1000U < nr->weight * 586U) { /* LL */ + if ((uint64_t) WEIGHT(nrl) * 1000U < (uint64_t) nr->weight * 586U) { /* RR */ /* Rotate |n| left. */ tree_node_rot_left(tree, n); nr->weight = (n->weight = WEIGHT(n->llink) + WEIGHT(n->rlink)) + @@ -193,11 +205,11 @@ fixup(wb_tree* tree, wb_node* n) (nr->weight = WEIGHT(b) + WEIGHT(nr->rlink)); rotations += 2; } - } else if (weight * 1000U > n->weight * 707U) { + } else if ((uint64_t) weight * 1000U > (uint64_t) n->weight * 707U) { wb_node* nl = n->llink; ASSERT(nl != NULL); weight = WEIGHT(nl->llink); - if (weight * 1000U > nl->weight * 414U) { /* RR */ + if ((uint64_t) weight * 1000U > (uint64_t) nl->weight * 414U) { /* LL */ tree_node_rot_right(tree, n); n->weight = WEIGHT(n->llink) + WEIGHT(n->rlink); @@ -290,7 +302,7 @@ remove_node(wb_tree* tree, wb_node* node) node = out; } ASSERT(!node->llink || !node->rlink); - /* Splice in the successor, if any. */ + /* Splice in the node's only child, if any. */ wb_node* child = node->llink ? node->llink : node->rlink; wb_node* parent = node->parent; if (child) @@ -375,7 +387,7 @@ node_new(void* key) static bool node_verify(const wb_tree* tree, const wb_node* parent, const wb_node* node, - unsigned *weight) + unsigned *weight, const void* lower, const void* upper) { if (!parent) { VERIFY(tree->root == node); @@ -384,23 +396,23 @@ node_verify(const wb_tree* tree, const wb_node* parent, const wb_node* node, } if (node) { VERIFY(node->parent == parent); - if (parent) { - if (parent->llink == node) { - VERIFY(tree->cmp_func(parent->key, node->key) > 0); - } else { - ASSERT(parent->rlink == node); - VERIFY(tree->cmp_func(parent->key, node->key) < 0); - } - } + /* Global BST ordering. Comparing a node only against its immediate + * parent would accept an entire subtree placed on the wrong side: + * root 5, 5->llink 3, 3->rlink 7 passes every parent-local check yet + * makes key 7 unreachable from the root. */ + if (lower) + VERIFY(tree->cmp_func(lower, node->key) < 0); + if (upper) + VERIFY(tree->cmp_func(upper, node->key) > 0); unsigned lweight, rweight; - if (!node_verify(tree, node, node->llink, &lweight) || - !node_verify(tree, node, node->rlink, &rweight)) + if (!node_verify(tree, node, node->llink, &lweight, lower, node->key) || + !node_verify(tree, node, node->rlink, &rweight, node->key, upper)) return false; VERIFY(WEIGHT(node->llink) == lweight); VERIFY(WEIGHT(node->rlink) == rweight); VERIFY(node->weight == lweight + rweight); - VERIFY(lweight * 1000U >= node->weight * 292U); - VERIFY(lweight * 1000U <= node->weight * 708U); + VERIFY((uint64_t) lweight * 1000U >= (uint64_t) node->weight * 292U); + VERIFY((uint64_t) lweight * 1000U <= (uint64_t) node->weight * 708U); *weight = lweight + rweight; } else { *weight = 1; @@ -417,8 +429,10 @@ wb_tree_verify(const wb_tree* tree) } else { VERIFY(tree->count == 0); } + /* The weight relationships are checked inside node_verify(); the + * top-level weight itself is already covered by the count check above. */ unsigned root_weight; - return node_verify(tree, NULL, tree->root, &root_weight); + return node_verify(tree, NULL, tree->root, &root_weight, NULL, NULL); } wb_itor* diff --git a/unit_tests.c b/unit_tests.c index e3516ae6..0c8aada4 100644 --- a/unit_tests.c +++ b/unit_tests.c @@ -45,7 +45,14 @@ void test_basic_splay_tree(void); void test_basic_treap(void); void test_basic_weight_balanced_tree(void); void test_search(dict *dct, dict_itor *itor, const char *key, const char *value); -void test_closest_lookup(dict *dct, unsigned nkeys, bool keys_sorted); +void test_closest_lookup(dict *dct, const struct key_info *keys, unsigned nkeys); +void test_traverse(void); +void test_itor_nextn_prevn(void); +void test_itor_invalidate(void); +void test_itor_compare_unsorted(void); +void test_select_null_out(void); +void test_delete_func(void); +void test_comparators(void); void test_primes_geq(void); void test_version_string(void); @@ -61,6 +68,13 @@ static CU_TestInfo basic_tests[] = { TEST_FUNC(test_basic_splay_tree), TEST_FUNC(test_basic_treap), TEST_FUNC(test_basic_weight_balanced_tree), + TEST_FUNC(test_traverse), + TEST_FUNC(test_itor_nextn_prevn), + TEST_FUNC(test_itor_invalidate), + TEST_FUNC(test_itor_compare_unsorted), + TEST_FUNC(test_select_null_out), + TEST_FUNC(test_delete_func), + TEST_FUNC(test_comparators), TEST_FUNC(test_primes_geq), TEST_FUNC(test_version_string), CU_TEST_INFO_NULL @@ -79,23 +93,34 @@ static void* custom_malloc(size_t n) { size_t* p = malloc(sizeof(size_t) + n); - assert(p); + if (!p) { + /* Must not rely on assert(): it disappears under -DNDEBUG, and the + * store to p[0] below would then write through a NULL pointer. */ + fprintf(stderr, "custom_malloc: out of memory allocating %zu bytes\n", n); + abort(); + } bytes_malloced += n; p[0] = n; + /* NOTE: the returned block is only sizeof(size_t)-aligned, which is weaker + * than malloc()'s guarantee. That is acceptable here because libdict only + * ever stores pointer-sized payloads in the memory it allocates. */ return &p[1]; } static void custom_free(void* p) { - assert(p); + if (!p) { + fprintf(stderr, "custom_free: NULL pointer\n"); + abort(); + } size_t* sp = p; bytes_malloced -= sp[-1]; free(sp - 1); } int -main() +main(void) { dict_malloc_func = custom_malloc; dict_free_func = custom_free; @@ -269,64 +294,189 @@ test_search(dict *dct, dict_itor *itor, const char *key, const char *value) } } +/* Identify a container by its vtable. The slots hold file-local thunks with + * the exact slot signatures rather than the public typed functions (calling a + * function through an incompatible function-pointer type is undefined + * behaviour), so a slot can no longer be compared against a public function. + * Every dict of a given type shares one static vtable, so compare against the + * vtable of a throwaway instance instead. */ +static const dict_vtable* +reference_vtable(dict *dct) +{ + CU_ASSERT_PTR_NOT_NULL_FATAL(dct); + const dict_vtable* vtable = dct->_vtable; + dict_free(dct, NULL); + return vtable; +} + +/* True if `dct` is a chained hashtable. */ +static bool +is_hashtable(const dict *dct) +{ + static const dict_vtable* vtable = NULL; + if (!vtable) + vtable = reference_vtable(hashtable_dict_new(dict_str_cmp, + dict_str_hash, 1)); + return dct->_vtable == vtable; +} + +/* True if `dct` is an open-addressing hashtable. */ +static bool +is_hashtable2(const dict *dct) +{ + static const dict_vtable* vtable = NULL; + if (!vtable) + vtable = reference_vtable(hashtable2_dict_new(dict_str_cmp, + dict_str_hash, 1)); + return dct->_vtable == vtable; +} + +/* Compute the expected le/lt/ge/gt answers for `probe` over the first `nkeys` + * entries of `keys` -- i.e. exactly the contents of the dictionary under test. + * A NULL result key means "no such element". */ +static void +compute_closest(const struct key_info *keys, unsigned nkeys, const char *probe, + struct closest_lookup_info *out) +{ + memset(out, 0, sizeof(*out)); + out->key = probe; + for (unsigned i = 0; i < nkeys; ++i) { + const int cmp = strcmp(keys[i].key, probe); + if (cmp <= 0 && (!out->le_key || strcmp(keys[i].key, out->le_key) > 0)) { + out->le_key = keys[i].key; + out->le_val = keys[i].value; + } + if (cmp < 0 && (!out->lt_key || strcmp(keys[i].key, out->lt_key) > 0)) { + out->lt_key = keys[i].key; + out->lt_val = keys[i].value; + } + if (cmp >= 0 && (!out->ge_key || strcmp(keys[i].key, out->ge_key) < 0)) { + out->ge_key = keys[i].key; + out->ge_val = keys[i].value; + } + if (cmp > 0 && (!out->gt_key || strcmp(keys[i].key, out->gt_key) < 0)) { + out->gt_key = keys[i].key; + out->gt_val = keys[i].value; + } + } +} + +static void +check_expectation_equal(const char *computed_key, const char *computed_val, + const char *table_key, const char *table_val) +{ + if (table_key == NULL) { + CU_ASSERT_PTR_NULL(computed_key); + CU_ASSERT_PTR_NULL(computed_val); + } else { + CU_ASSERT_PTR_NOT_NULL(computed_key); + if (computed_key) { + CU_ASSERT_STRING_EQUAL(computed_key, table_key); + CU_ASSERT_STRING_EQUAL(computed_val, table_val); + } + } +} + void -test_closest_lookup(dict *dct, unsigned nkeys, bool keys_sorted) +test_closest_lookup(dict *dct, const struct key_info *keys, unsigned nkeys) { - if (dict_is_sorted(dct) && keys_sorted) { + const bool sorted = dict_is_sorted(dct); + dict_itor* itor = dict_itor_new(dct); + + /* These vtable expectations are properties of the container type, not of + * the individual lookup being performed, so they are checked once. */ + if (sorted) { + /* Every sorted container must provide the near-search operations, on + * both the dictionary and its iterators. */ + CU_ASSERT_PTR_NOT_NULL(dct->_vtable->search_le); + CU_ASSERT_PTR_NOT_NULL(dct->_vtable->search_lt); + CU_ASSERT_PTR_NOT_NULL(dct->_vtable->search_ge); + CU_ASSERT_PTR_NOT_NULL(dct->_vtable->search_gt); + CU_ASSERT_PTR_NOT_NULL(itor->_vtable->search_le); + CU_ASSERT_PTR_NOT_NULL(itor->_vtable->search_lt); + CU_ASSERT_PTR_NOT_NULL(itor->_vtable->search_ge); + CU_ASSERT_PTR_NOT_NULL(itor->_vtable->search_gt); + /* Every sorted container provides select. */ + CU_ASSERT_PTR_NOT_NULL(dct->_vtable->select); + } else { + /* Unsorted containers legitimately lack these operations. */ + CU_ASSERT_PTR_NULL(dct->_vtable->search_le); + CU_ASSERT_PTR_NULL(dct->_vtable->search_lt); + CU_ASSERT_PTR_NULL(dct->_vtable->search_ge); + CU_ASSERT_PTR_NULL(dct->_vtable->search_gt); + CU_ASSERT_PTR_NULL(itor->_vtable->search_le); + CU_ASSERT_PTR_NULL(itor->_vtable->search_lt); + CU_ASSERT_PTR_NULL(itor->_vtable->search_ge); + CU_ASSERT_PTR_NULL(itor->_vtable->search_gt); + CU_ASSERT_PTR_NULL(dct->_vtable->select); + } + + if (sorted && dct->_vtable->select) { + /* Build the expected ascending order of whatever keys the dictionary + * currently holds, which is the first nkeys entries of `keys`. */ + const struct key_info *order[NUM_SORTED_KEYS]; + for (unsigned i = 0; i < nkeys; ++i) + order[i] = &keys[i]; + for (unsigned i = 0; i + 1 < nkeys; ++i) { + unsigned min = i; + for (unsigned j = i + 1; j < nkeys; ++j) + if (strcmp(order[j]->key, order[min]->key) < 0) + min = j; + const struct key_info *tmp = order[i]; + order[i] = order[min]; + order[min] = tmp; + } for (unsigned i = 0; i < nkeys; ++i) { const void* key = NULL; void* datum = NULL; - if (dct->_vtable->select) { - CU_ASSERT_TRUE(dict_select(dct, i, &key, &datum)); - CU_ASSERT_EQUAL(key, sorted_keys[i].key); - CU_ASSERT_EQUAL(datum, sorted_keys[i].value); - } + CU_ASSERT_TRUE(dict_select(dct, i, &key, &datum)); + CU_ASSERT_EQUAL(key, order[i]->key); + CU_ASSERT_EQUAL(datum, order[i]->value); } - const void* key = NULL; - void* datum = NULL; + const void* key = (const void*)"unwritten"; + void* datum = (void*)"unwritten"; CU_ASSERT_FALSE(dict_select(dct, nkeys, &key, &datum)); CU_ASSERT_EQUAL(key, (const void*)NULL); CU_ASSERT_PTR_NULL(datum); } - dict_itor* itor = dict_itor_new(dct); for (unsigned i = 0; i < NUM_CLOSEST_LOOKUP_INFOS; i++) { const struct closest_lookup_info* const info = &closest_lookup_infos[i]; - if (!dict_is_sorted(dct)) { - CU_ASSERT_PTR_NULL(dct->_vtable->search_le); - CU_ASSERT_PTR_NULL(dct->_vtable->search_lt); - CU_ASSERT_PTR_NULL(dct->_vtable->search_ge); - CU_ASSERT_PTR_NULL(dct->_vtable->search_gt); - CU_ASSERT_PTR_NULL(itor->_vtable->search_le); - CU_ASSERT_PTR_NULL(itor->_vtable->search_lt); - CU_ASSERT_PTR_NULL(itor->_vtable->search_ge); - CU_ASSERT_PTR_NULL(itor->_vtable->search_gt); - const void* key = NULL; - void* datum = NULL; - CU_ASSERT_FALSE(dict_select(dct, i, &key, &datum)); - CU_ASSERT_EQUAL(key, (const void*)NULL); - CU_ASSERT_PTR_NULL(datum); + if (!sorted) { + /* The near-search operations must report "not found" rather than + * misbehaving on containers that do not support them. */ CU_ASSERT_PTR_NULL(dict_search_le(dct, info->key)); CU_ASSERT_PTR_NULL(dict_search_lt(dct, info->key)); CU_ASSERT_PTR_NULL(dict_search_ge(dct, info->key)); CU_ASSERT_PTR_NULL(dict_search_gt(dct, info->key)); + CU_ASSERT_FALSE(dict_itor_search_le(itor, info->key)); + CU_ASSERT_FALSE(dict_itor_search_lt(itor, info->key)); + CU_ASSERT_FALSE(dict_itor_search_ge(itor, info->key)); + CU_ASSERT_FALSE(dict_itor_search_gt(itor, info->key)); continue; } - if (nkeys < NUM_SORTED_KEYS) - continue; - if (info->le_key) { - if (dct->_vtable->search_le) { - CU_ASSERT_PTR_NOT_NULL(dict_search_le(dct, info->key)); - void **datum = dict_search_le(dct, info->key); - CU_ASSERT_TRUE(datum != NULL && *datum != NULL && strcmp(*datum, info->le_val) == 0); - } - if (itor->_vtable->search_le) { - CU_ASSERT_TRUE(dict_itor_search_le(itor, info->key)); - CU_ASSERT_TRUE(dict_itor_valid(itor)); - CU_ASSERT_STRING_EQUAL(dict_itor_key(itor), info->le_key); - void **datum = dict_itor_datum(itor); - CU_ASSERT_TRUE(datum != NULL && *datum != NULL && strcmp(*datum, info->le_val) == 0); - } + + /* Expectations for the keys actually present, so that this block runs + * for every nkeys, not just for the complete key set. */ + struct closest_lookup_info expect; + compute_closest(keys, nkeys, info->key, &expect); + if (nkeys == NUM_SORTED_KEYS) { + /* Cross-check the computation against the hand-written table. */ + check_expectation_equal(expect.le_key, expect.le_val, info->le_key, info->le_val); + check_expectation_equal(expect.lt_key, expect.lt_val, info->lt_key, info->lt_val); + check_expectation_equal(expect.ge_key, expect.ge_val, info->ge_key, info->ge_val); + check_expectation_equal(expect.gt_key, expect.gt_val, info->gt_key, info->gt_val); + } + + if (expect.le_key) { + void **datum = dict_search_le(dct, info->key); + CU_ASSERT_PTR_NOT_NULL(datum); + CU_ASSERT_TRUE(datum != NULL && *datum != NULL && strcmp(*datum, expect.le_val) == 0); + CU_ASSERT_TRUE(dict_itor_search_le(itor, info->key)); + CU_ASSERT_TRUE(dict_itor_valid(itor)); + CU_ASSERT_STRING_EQUAL(dict_itor_key(itor), expect.le_key); + datum = dict_itor_datum(itor); + CU_ASSERT_TRUE(datum != NULL && *datum != NULL && strcmp(*datum, expect.le_val) == 0); } else { CU_ASSERT_PTR_NULL(dict_search_le(dct, info->key)); CU_ASSERT_FALSE(dict_itor_search_le(itor, info->key)); @@ -334,19 +484,15 @@ test_closest_lookup(dict *dct, unsigned nkeys, bool keys_sorted) CU_ASSERT_PTR_NULL(dict_itor_key(itor)); CU_ASSERT_PTR_NULL(dict_itor_datum(itor)); } - if (info->lt_key) { - if (dct->_vtable->search_lt) { - CU_ASSERT_PTR_NOT_NULL(dict_search_lt(dct, info->key)); - void** datum = dict_search_lt(dct, info->key); - CU_ASSERT_TRUE(datum != NULL && *datum != NULL && strcmp(*datum, info->lt_val) == 0); - } - if (itor->_vtable->search_lt) { - CU_ASSERT_TRUE(dict_itor_search_lt(itor, info->key)); - CU_ASSERT_TRUE(dict_itor_valid(itor)); - CU_ASSERT_STRING_EQUAL(dict_itor_key(itor), info->lt_key); - void** datum = dict_itor_datum(itor); - CU_ASSERT_TRUE(datum != NULL && *datum != NULL && strcmp(*datum, info->lt_val) == 0); - } + if (expect.lt_key) { + void **datum = dict_search_lt(dct, info->key); + CU_ASSERT_PTR_NOT_NULL(datum); + CU_ASSERT_TRUE(datum != NULL && *datum != NULL && strcmp(*datum, expect.lt_val) == 0); + CU_ASSERT_TRUE(dict_itor_search_lt(itor, info->key)); + CU_ASSERT_TRUE(dict_itor_valid(itor)); + CU_ASSERT_STRING_EQUAL(dict_itor_key(itor), expect.lt_key); + datum = dict_itor_datum(itor); + CU_ASSERT_TRUE(datum != NULL && *datum != NULL && strcmp(*datum, expect.lt_val) == 0); } else { CU_ASSERT_PTR_NULL(dict_search_lt(dct, info->key)); CU_ASSERT_FALSE(dict_itor_search_lt(itor, info->key)); @@ -354,19 +500,15 @@ test_closest_lookup(dict *dct, unsigned nkeys, bool keys_sorted) CU_ASSERT_PTR_NULL(dict_itor_key(itor)); CU_ASSERT_PTR_NULL(dict_itor_datum(itor)); } - if (info->ge_key) { - if (dct->_vtable->search_ge) { - CU_ASSERT_PTR_NOT_NULL(dict_search_ge(dct, info->key)); - void** datum = dict_search_ge(dct, info->key); - CU_ASSERT_TRUE(datum != NULL && *datum != NULL && strcmp(*datum, info->ge_val) == 0); - } - if (itor->_vtable->search_ge) { - CU_ASSERT_TRUE(dict_itor_search_ge(itor, info->key)); - CU_ASSERT_TRUE(dict_itor_valid(itor)); - CU_ASSERT_STRING_EQUAL(dict_itor_key(itor), info->ge_key); - void** datum = dict_itor_datum(itor); - CU_ASSERT_TRUE(datum != NULL && *datum != NULL && strcmp(*datum, info->ge_val) == 0); - } + if (expect.ge_key) { + void **datum = dict_search_ge(dct, info->key); + CU_ASSERT_PTR_NOT_NULL(datum); + CU_ASSERT_TRUE(datum != NULL && *datum != NULL && strcmp(*datum, expect.ge_val) == 0); + CU_ASSERT_TRUE(dict_itor_search_ge(itor, info->key)); + CU_ASSERT_TRUE(dict_itor_valid(itor)); + CU_ASSERT_STRING_EQUAL(dict_itor_key(itor), expect.ge_key); + datum = dict_itor_datum(itor); + CU_ASSERT_TRUE(datum != NULL && *datum != NULL && strcmp(*datum, expect.ge_val) == 0); } else { CU_ASSERT_PTR_NULL(dict_search_ge(dct, info->key)); CU_ASSERT_FALSE(dict_itor_search_ge(itor, info->key)); @@ -374,19 +516,15 @@ test_closest_lookup(dict *dct, unsigned nkeys, bool keys_sorted) CU_ASSERT_PTR_NULL(dict_itor_key(itor)); CU_ASSERT_PTR_NULL(dict_itor_datum(itor)); } - if (info->gt_key) { - if (dct->_vtable->search_gt) { - CU_ASSERT_PTR_NOT_NULL(dict_search_gt(dct, info->key)); - void** datum = dict_search_gt(dct, info->key); - CU_ASSERT_TRUE(datum != NULL && *datum != NULL && strcmp(*datum, info->gt_val) == 0); - } - if (itor->_vtable->search_gt) { - CU_ASSERT_TRUE(dict_itor_search_gt(itor, info->key)); - CU_ASSERT_TRUE(dict_itor_valid(itor)); - CU_ASSERT_STRING_EQUAL(dict_itor_key(itor), info->gt_key); - void** datum = dict_itor_datum(itor); - CU_ASSERT_TRUE(datum != NULL && *datum != NULL && strcmp(*datum, info->gt_val) == 0); - } + if (expect.gt_key) { + void **datum = dict_search_gt(dct, info->key); + CU_ASSERT_PTR_NOT_NULL(datum); + CU_ASSERT_TRUE(datum != NULL && *datum != NULL && strcmp(*datum, expect.gt_val) == 0); + CU_ASSERT_TRUE(dict_itor_search_gt(itor, info->key)); + CU_ASSERT_TRUE(dict_itor_valid(itor)); + CU_ASSERT_STRING_EQUAL(dict_itor_key(itor), expect.gt_key); + datum = dict_itor_datum(itor); + CU_ASSERT_TRUE(datum != NULL && *datum != NULL && strcmp(*datum, expect.gt_val) == 0); } else { CU_ASSERT_PTR_NULL(dict_search_gt(dct, info->key)); CU_ASSERT_FALSE(dict_itor_search_gt(itor, info->key)); @@ -428,10 +566,9 @@ void test_basic(dict *dct, const struct key_info *keys, const unsigned nkeys, bo } CU_ASSERT_EQUAL(dict_count(dct), nkeys); - if (dct->_vtable->insert == (dict_insert_func)hashtable_insert || - dct->_vtable->insert == (dict_insert_func)hashtable2_insert) { + if (is_hashtable(dct) || is_hashtable2(dct)) { /* Verify that hashtable_resize works as expected. */ - if (dct->_vtable->insert == (dict_insert_func)hashtable_insert) { + if (is_hashtable(dct)) { CU_ASSERT_TRUE(hashtable_resize(dict_private(dct), 3)); } else { CU_ASSERT_TRUE(hashtable2_resize(dict_private(dct), @@ -487,7 +624,7 @@ void test_basic(dict *dct, const struct key_info *keys, const unsigned nkeys, bo } } - char *last_key = NULL; + const char *last_key = NULL; unsigned n = 0; for (dict_itor_first(itor); dict_itor_valid(itor); dict_itor_next(itor)) { CU_ASSERT_PTR_NOT_NULL(dict_itor_key(itor)); @@ -630,7 +767,7 @@ void test_basic(dict *dct, const struct key_info *keys, const unsigned nkeys, bo } CU_ASSERT_EQUAL(dict_count(dct), nkeys); - test_closest_lookup(dct, nkeys, keys_sorted); + test_closest_lookup(dct, keys, nkeys); for (unsigned i = 0; i < nkeys; ++i) { CU_ASSERT_TRUE(dict_itor_search(itor, keys[i].key)); @@ -788,6 +925,500 @@ void test_basic_weight_balanced_tree() } } +/* Every container type covered by the basic tests, so that the tests below can + * be run against all of them. */ +static dict *new_hashtable_1bucket(void) +{ return hashtable_dict_new(dict_str_cmp, dict_str_hash, 1); } +static dict *new_hashtable_nbuckets(void) +{ return hashtable_dict_new(dict_str_cmp, dict_str_hash, 7); } +static dict *new_hashtable2_1bucket(void) +{ return hashtable2_dict_new(dict_str_cmp, dict_str_hash, 1); } +static dict *new_hashtable2_nbuckets(void) +{ return hashtable2_dict_new(dict_str_cmp, dict_str_hash, 7); } +static dict *new_hb_tree(void) { return hb_dict_new(dict_str_cmp); } +static dict *new_pr_tree(void) { return pr_dict_new(dict_str_cmp); } +static dict *new_rb_tree(void) { return rb_dict_new(dict_str_cmp); } +static dict *new_skiplist(void) { return skiplist_dict_new(dict_str_cmp, 13); } +static dict *new_sp_tree(void) { return sp_dict_new(dict_str_cmp); } +static dict *new_tr_tree(void) { return tr_dict_new(dict_str_cmp, NULL); } +static dict *new_wb_tree(void) { return wb_dict_new(dict_str_cmp); } + +struct container_info { + const char *name; + dict *(*new_dict)(void); +}; + +static const struct container_info containers[] = { + { "hashtable_1bucket", new_hashtable_1bucket }, + { "hashtable_nbuckets", new_hashtable_nbuckets }, + { "hashtable2_1bucket", new_hashtable2_1bucket }, + { "hashtable2_nbuckets", new_hashtable2_nbuckets }, + { "hb_tree", new_hb_tree }, + { "pr_tree", new_pr_tree }, + { "rb_tree", new_rb_tree }, + { "skiplist", new_skiplist }, + { "sp_tree", new_sp_tree }, + { "tr_tree", new_tr_tree }, + { "wb_tree", new_wb_tree }, +}; +#define NUM_CONTAINERS (sizeof(containers) / sizeof(containers[0])) + +/* Insert the first nkeys entries of `keys` into `dct`. */ +static void +fill_dict(dict *dct, const struct key_info *keys, unsigned nkeys) +{ + for (unsigned i = 0; i < nkeys; ++i) { + dict_insert_result result = dict_insert(dct, keys[i].key); + CU_ASSERT_TRUE(result.inserted); + CU_ASSERT_PTR_NOT_NULL(result.datum_ptr); + if (result.datum_ptr) + *result.datum_ptr = keys[i].value; + } + CU_ASSERT_EQUAL(dict_count(dct), nkeys); +} + +struct traverse_state { + dict *dct; + bool sorted; + unsigned visits; + unsigned stop_after; /* 0 means never stop early */ + const char *last_key; +}; + +static bool +traverse_visit(const void *key, void *datum, void *user_data) +{ + struct traverse_state *state = user_data; + + ++state->visits; + CU_ASSERT_PTR_NOT_NULL(key); + /* Every visited pair must agree with what a search for the key yields. */ + void **search = dict_search(state->dct, key); + CU_ASSERT_PTR_NOT_NULL(search); + if (search) + CU_ASSERT_PTR_EQUAL(*search, datum); + if (state->sorted) { + if (state->last_key) + CU_ASSERT_TRUE(strcmp(state->last_key, key) < 0); + state->last_key = key; + } + if (state->stop_after && state->visits >= state->stop_after) + return false; + return true; +} + +void test_traverse() +{ + for (unsigned c = 0; c < NUM_CONTAINERS; ++c) { + for (unsigned nkeys = 0; nkeys <= NUM_SORTED_KEYS; ++nkeys) { + for (unsigned pass = 0; pass < 2; ++pass) { + const struct key_info *keys = pass ? sorted_keys : unsorted_keys; + dict *dct = containers[c].new_dict(); + CU_ASSERT_PTR_NOT_NULL_FATAL(dct); + fill_dict(dct, keys, nkeys); + + struct traverse_state state = { + .dct = dct, .sorted = dict_is_sorted(dct) + }; + CU_ASSERT_EQUAL(dict_traverse(dct, traverse_visit, &state), nkeys); + CU_ASSERT_EQUAL(state.visits, nkeys); + + /* A visit function that returns false after k elements must + * stop the traversal and make dict_traverse() return k. */ + for (unsigned k = 1; k <= nkeys; ++k) { + struct traverse_state stop_state = { + .dct = dct, .sorted = dict_is_sorted(dct), .stop_after = k + }; + CU_ASSERT_EQUAL(dict_traverse(dct, traverse_visit, &stop_state), k); + CU_ASSERT_EQUAL(stop_state.visits, k); + } + + CU_ASSERT_EQUAL(dict_free(dct, NULL), nkeys); + CU_ASSERT_EQUAL(bytes_malloced, 0); + } + } + } +} + +void test_itor_nextn_prevn() +{ + static const unsigned key_counts[] = { 0, 1, 2, 3, 7, NUM_SORTED_KEYS }; + + for (unsigned c = 0; c < NUM_CONTAINERS; ++c) { + for (unsigned ki = 0; ki < sizeof(key_counts) / sizeof(key_counts[0]); ++ki) { + const unsigned nkeys = key_counts[ki]; + dict *dct = containers[c].new_dict(); + CU_ASSERT_PTR_NOT_NULL_FATAL(dct); + fill_dict(dct, unsorted_keys, nkeys); + + for (unsigned n = 0; n <= nkeys + 1; ++n) { + dict_itor *a = dict_itor_new(dct); + dict_itor *b = dict_itor_new(dct); + CU_ASSERT_PTR_NOT_NULL_FATAL(a); + CU_ASSERT_PTR_NOT_NULL_FATAL(b); + CU_ASSERT_PTR_NOT_NULL(a->_vtable->compare); + + /* nextn(n) from the first element must land where n successive + * next() calls land, and agree on the returned bool. */ + CU_ASSERT_EQUAL(dict_itor_first(a), nkeys > 0); + CU_ASSERT_EQUAL(dict_itor_first(b), nkeys > 0); + const bool a_ret = dict_itor_nextn(a, n); + bool b_ret = dict_itor_valid(b); + for (unsigned j = 0; j < n; ++j) { + b_ret = dict_itor_next(b); + if (!b_ret) + break; + } + CU_ASSERT_EQUAL(a_ret, b_ret); + CU_ASSERT_EQUAL(dict_itor_valid(a), dict_itor_valid(b)); + CU_ASSERT_EQUAL(dict_itor_compare(a, b), 0); + CU_ASSERT_EQUAL(dict_itor_compare(b, a), 0); + if (dict_itor_valid(a)) { + CU_ASSERT_PTR_EQUAL(dict_itor_key(a), dict_itor_key(b)); + CU_ASSERT_EQUAL(a_ret, true); + } + + /* Same for prevn(n) from the last element. */ + CU_ASSERT_EQUAL(dict_itor_last(a), nkeys > 0); + CU_ASSERT_EQUAL(dict_itor_last(b), nkeys > 0); + const bool a_prev_ret = dict_itor_prevn(a, n); + bool b_prev_ret = dict_itor_valid(b); + for (unsigned j = 0; j < n; ++j) { + b_prev_ret = dict_itor_prev(b); + if (!b_prev_ret) + break; + } + CU_ASSERT_EQUAL(a_prev_ret, b_prev_ret); + CU_ASSERT_EQUAL(dict_itor_valid(a), dict_itor_valid(b)); + CU_ASSERT_EQUAL(dict_itor_compare(a, b), 0); + CU_ASSERT_EQUAL(dict_itor_compare(b, a), 0); + if (dict_itor_valid(a)) { + CU_ASSERT_PTR_EQUAL(dict_itor_key(a), dict_itor_key(b)); + CU_ASSERT_EQUAL(a_prev_ret, true); + } + + dict_itor_free(a); + dict_itor_free(b); + } + CU_ASSERT_EQUAL(dict_free(dct, NULL), nkeys); + CU_ASSERT_EQUAL(bytes_malloced, 0); + } + } +} + +void test_itor_invalidate() +{ + for (unsigned c = 0; c < NUM_CONTAINERS; ++c) { + dict *dct = containers[c].new_dict(); + CU_ASSERT_PTR_NOT_NULL_FATAL(dct); + fill_dict(dct, unsorted_keys, NUM_SORTED_KEYS); + + dict_itor *itor = dict_itor_new(dct); + CU_ASSERT_PTR_NOT_NULL_FATAL(itor); + CU_ASSERT_TRUE(dict_itor_first(itor)); + CU_ASSERT_TRUE(dict_itor_valid(itor)); + dict_itor_invalidate(itor); + CU_ASSERT_FALSE(dict_itor_valid(itor)); + CU_ASSERT_PTR_NULL(dict_itor_key(itor)); + CU_ASSERT_PTR_NULL(dict_itor_datum(itor)); + + /* Invalidating an already invalid iterator is harmless. */ + dict_itor_invalidate(itor); + CU_ASSERT_FALSE(dict_itor_valid(itor)); + CU_ASSERT_PTR_NULL(dict_itor_key(itor)); + CU_ASSERT_PTR_NULL(dict_itor_datum(itor)); + + /* The iterator is still usable afterwards. */ + CU_ASSERT_TRUE(dict_itor_last(itor)); + CU_ASSERT_TRUE(dict_itor_valid(itor)); + CU_ASSERT_PTR_NOT_NULL(dict_itor_key(itor)); + + dict_itor_free(itor); + CU_ASSERT_EQUAL(dict_free(dct, NULL), NUM_SORTED_KEYS); + CU_ASSERT_EQUAL(bytes_malloced, 0); + } +} + +void test_itor_compare_unsorted() +{ + for (unsigned c = 0; c < NUM_CONTAINERS; ++c) { + for (unsigned nkeys = 0; nkeys <= 5; ++nkeys) { + dict *dct = containers[c].new_dict(); + CU_ASSERT_PTR_NOT_NULL_FATAL(dct); + /* Ordering assertions specific to sorted containers live in + * test_basic(); this test covers what holds for every container, + * sorted or not. */ + fill_dict(dct, unsorted_keys, nkeys); + + dict_itor *first = dict_itor_new(dct); + dict_itor *last = dict_itor_new(dct); + dict_itor *invalid = dict_itor_new(dct); + CU_ASSERT_PTR_NOT_NULL_FATAL(first); + CU_ASSERT_PTR_NOT_NULL_FATAL(last); + CU_ASSERT_PTR_NOT_NULL_FATAL(invalid); + /* Every container -- sorted or not -- must implement compare. */ + CU_ASSERT_PTR_NOT_NULL(first->_vtable->compare); + + CU_ASSERT_EQUAL(dict_itor_first(first), nkeys > 0); + CU_ASSERT_EQUAL(dict_itor_last(last), nkeys > 0); + CU_ASSERT_FALSE(dict_itor_valid(invalid)); + + /* An iterator always equals itself. */ + CU_ASSERT_EQUAL(dict_itor_compare(first, first), 0); + CU_ASSERT_EQUAL(dict_itor_compare(last, last), 0); + CU_ASSERT_EQUAL(dict_itor_compare(invalid, invalid), 0); + + if (nkeys == 0) { + CU_ASSERT_EQUAL(dict_itor_compare(first, last), 0); + CU_ASSERT_EQUAL(dict_itor_compare(last, first), 0); + } else if (nkeys == 1) { + CU_ASSERT_EQUAL(dict_itor_compare(first, last), 0); + CU_ASSERT_EQUAL(dict_itor_compare(last, first), 0); + } else { + /* Distinct elements compare nonzero with opposite signs. */ + const int cmp = dict_itor_compare(first, last); + const int rcmp = dict_itor_compare(last, first); + CU_ASSERT_NOT_EQUAL(cmp, 0); + CU_ASSERT_TRUE((cmp < 0) == (rcmp > 0)); + /* Repeating the comparison gives the same answer. */ + CU_ASSERT_EQUAL(dict_itor_compare(first, last), cmp); + } + + if (nkeys > 0) { + /* An invalid iterator compares consistently against a valid + * one, in both directions. */ + const int cmp = dict_itor_compare(invalid, first); + const int rcmp = dict_itor_compare(first, invalid); + CU_ASSERT_NOT_EQUAL(cmp, 0); + CU_ASSERT_NOT_EQUAL(rcmp, 0); + CU_ASSERT_TRUE((cmp < 0) == (rcmp > 0)); + + /* Invalidating an iterator makes it compare like a freshly + * created (invalid) one. */ + dict_itor_invalidate(last); + CU_ASSERT_EQUAL(dict_itor_compare(last, invalid), 0); + CU_ASSERT_EQUAL(dict_itor_compare(invalid, last), 0); + CU_ASSERT_EQUAL(dict_itor_compare(last, first), cmp); + CU_ASSERT_EQUAL(dict_itor_compare(first, last), rcmp); + } + + dict_itor_free(first); + dict_itor_free(last); + dict_itor_free(invalid); + CU_ASSERT_EQUAL(dict_free(dct, NULL), nkeys); + CU_ASSERT_EQUAL(bytes_malloced, 0); + } + } +} + +void test_select_null_out() +{ + for (unsigned c = 0; c < NUM_CONTAINERS; ++c) { + for (unsigned nkeys = 0; nkeys <= 5; ++nkeys) { + dict *dct = containers[c].new_dict(); + CU_ASSERT_PTR_NOT_NULL_FATAL(dct); + if (!dict_is_sorted(dct)) { + dict_free(dct, NULL); + continue; + } + fill_dict(dct, unsorted_keys, nkeys); + const bool has_select = dct->_vtable->select != NULL; + + for (unsigned n = 0; n <= nkeys + 1; ++n) { + const bool expected = has_select && n < nkeys; + + /* Both out-parameters. */ + const void *key = NULL; + void *datum = NULL; + CU_ASSERT_EQUAL(dict_select(dct, n, &key, &datum), expected); + if (expected) { + CU_ASSERT_PTR_NOT_NULL(key); + CU_ASSERT_PTR_NOT_NULL(datum); + } else { + CU_ASSERT_PTR_NULL(key); + CU_ASSERT_PTR_NULL(datum); + } + + /* Only the datum requested. */ + void *datum_only = NULL; + CU_ASSERT_EQUAL(dict_select(dct, n, NULL, &datum_only), expected); + if (expected) { + CU_ASSERT_PTR_EQUAL(datum_only, datum); + } else { + CU_ASSERT_PTR_NULL(datum_only); + } + + /* Only the key requested. */ + const void *key_only = NULL; + CU_ASSERT_EQUAL(dict_select(dct, n, &key_only, NULL), expected); + if (expected) { + CU_ASSERT_PTR_EQUAL(key_only, key); + } else { + CU_ASSERT_PTR_NULL(key_only); + } + + /* Neither out-parameter. */ + CU_ASSERT_EQUAL(dict_select(dct, n, NULL, NULL), expected); + } + CU_ASSERT_EQUAL(dict_free(dct, NULL), nkeys); + CU_ASSERT_EQUAL(bytes_malloced, 0); + } + } +} + +#define NUM_DELETE_KEYS 16 + +static struct { + void *keys[NUM_DELETE_KEYS]; + void *data[NUM_DELETE_KEYS]; + unsigned seen[NUM_DELETE_KEYS]; + unsigned calls; +} delete_tracker; + +static void +tracking_delete_func(void *key, void *datum) +{ + ++delete_tracker.calls; + + bool found = false; + for (unsigned i = 0; i < NUM_DELETE_KEYS; ++i) { + if (delete_tracker.keys[i] == key) { + CU_ASSERT_PTR_EQUAL(delete_tracker.data[i], datum); + ++delete_tracker.seen[i]; + found = true; + break; + } + } + /* The delete function must only ever be handed pointers we inserted. */ + CU_ASSERT_TRUE(found); + + free(key); + free(datum); +} + +static char * +test_strdup(const char *str) +{ + const size_t len = strlen(str) + 1; + char *copy = malloc(len); + if (!copy) { + fprintf(stderr, "test_strdup: out of memory\n"); + abort(); + } + memcpy(copy, str, len); + return copy; +} + +void test_delete_func() +{ + for (unsigned c = 0; c < NUM_CONTAINERS; ++c) { + /* pass 0 exercises dict_clear(), pass 1 exercises dict_free(). */ + for (unsigned pass = 0; pass < 2; ++pass) { + dict *dct = containers[c].new_dict(); + CU_ASSERT_PTR_NOT_NULL_FATAL(dct); + + memset(&delete_tracker, 0, sizeof(delete_tracker)); + for (unsigned i = 0; i < NUM_DELETE_KEYS; ++i) { + char buf[32]; + snprintf(buf, sizeof(buf), "key%u", i); + char *key = test_strdup(buf); + snprintf(buf, sizeof(buf), "value%u", i); + char *datum = test_strdup(buf); + delete_tracker.keys[i] = key; + delete_tracker.data[i] = datum; + + dict_insert_result result = dict_insert(dct, key); + CU_ASSERT_TRUE(result.inserted); + CU_ASSERT_PTR_NOT_NULL_FATAL(result.datum_ptr); + *result.datum_ptr = datum; + } + CU_ASSERT_EQUAL(dict_count(dct), NUM_DELETE_KEYS); + CU_ASSERT_TRUE(dict_verify(dct)); + + if (pass == 0) { + CU_ASSERT_EQUAL(dict_clear(dct, tracking_delete_func), NUM_DELETE_KEYS); + CU_ASSERT_EQUAL(dict_count(dct), 0); + CU_ASSERT_TRUE(dict_verify(dct)); + CU_ASSERT_EQUAL(delete_tracker.calls, NUM_DELETE_KEYS); + /* An empty dictionary must not call the delete function. */ + CU_ASSERT_EQUAL(dict_free(dct, tracking_delete_func), 0); + CU_ASSERT_EQUAL(delete_tracker.calls, NUM_DELETE_KEYS); + } else { + CU_ASSERT_EQUAL(dict_free(dct, tracking_delete_func), NUM_DELETE_KEYS); + CU_ASSERT_EQUAL(delete_tracker.calls, NUM_DELETE_KEYS); + } + /* Exactly one call per element, with the pointers we inserted. */ + for (unsigned i = 0; i < NUM_DELETE_KEYS; ++i) + CU_ASSERT_EQUAL(delete_tracker.seen[i], 1); + CU_ASSERT_EQUAL(bytes_malloced, 0); + } + } +} + +static int +cmp_sign(int value) +{ + return (value > 0) - (value < 0); +} + +void test_comparators() +{ + const int ints[] = { INT_MIN, -3, -1, 0, 1, 3, INT_MAX }; + for (unsigned i = 0; i < sizeof(ints) / sizeof(ints[0]); ++i) + for (unsigned j = 0; j < sizeof(ints) / sizeof(ints[0]); ++j) + CU_ASSERT_EQUAL(cmp_sign(dict_int_cmp(&ints[i], &ints[j])), + (int)(i > j) - (int)(i < j)); + + const unsigned uints[] = { 0, 1, 3, INT_MAX, UINT_MAX }; + for (unsigned i = 0; i < sizeof(uints) / sizeof(uints[0]); ++i) + for (unsigned j = 0; j < sizeof(uints) / sizeof(uints[0]); ++j) + CU_ASSERT_EQUAL(cmp_sign(dict_uint_cmp(&uints[i], &uints[j])), + (int)(i > j) - (int)(i < j)); + + const long longs[] = { LONG_MIN, -3, -1, 0, 1, 3, LONG_MAX }; + for (unsigned i = 0; i < sizeof(longs) / sizeof(longs[0]); ++i) + for (unsigned j = 0; j < sizeof(longs) / sizeof(longs[0]); ++j) + CU_ASSERT_EQUAL(cmp_sign(dict_long_cmp(&longs[i], &longs[j])), + (int)(i > j) - (int)(i < j)); + + const unsigned long ulongs[] = { 0, 1, 3, LONG_MAX, ULONG_MAX }; + for (unsigned i = 0; i < sizeof(ulongs) / sizeof(ulongs[0]); ++i) + for (unsigned j = 0; j < sizeof(ulongs) / sizeof(ulongs[0]); ++j) + CU_ASSERT_EQUAL(cmp_sign(dict_ulong_cmp(&ulongs[i], &ulongs[j])), + (int)(i > j) - (int)(i < j)); + + int array[4]; + for (unsigned i = 0; i < sizeof(array) / sizeof(array[0]); ++i) + for (unsigned j = 0; j < sizeof(array) / sizeof(array[0]); ++j) + CU_ASSERT_EQUAL(cmp_sign(dict_ptr_cmp(&array[i], &array[j])), + (int)(i > j) - (int)(i < j)); + CU_ASSERT_EQUAL(dict_ptr_cmp(NULL, NULL), 0); + + /* dict_str_cmp() must agree in sign with strcmp(), which compares bytes as + * unsigned char -- so a high byte sorts above any ASCII character. */ + static const char *const strings[] = { + "", "a", "aa", "ab", "b", "A", "Z", "z", "\x7f", "\xC3\xA9", "\xC3", "\xff", + }; + for (unsigned i = 0; i < sizeof(strings) / sizeof(strings[0]); ++i) { + for (unsigned j = 0; j < sizeof(strings) / sizeof(strings[0]); ++j) { + CU_ASSERT_EQUAL(cmp_sign(dict_str_cmp(strings[i], strings[j])), + cmp_sign(strcmp(strings[i], strings[j]))); + } + } + CU_ASSERT_TRUE(dict_str_cmp("\xC3\xA9", "a") > 0); + CU_ASSERT_TRUE(dict_str_cmp("a", "\xC3\xA9") < 0); + + /* dict_str_hash() must be deterministic, and should differ for distinct + * short strings. */ + for (unsigned i = 0; i < sizeof(strings) / sizeof(strings[0]); ++i) + CU_ASSERT_EQUAL(dict_str_hash(strings[i]), dict_str_hash(strings[i])); + static const char *const hash_strings[] = { "a", "b", "c", "aa", "ab", "abc", "" }; + for (unsigned i = 0; i < sizeof(hash_strings) / sizeof(hash_strings[0]); ++i) + for (unsigned j = i + 1; j < sizeof(hash_strings) / sizeof(hash_strings[0]); ++j) + CU_ASSERT_NOT_EQUAL(dict_str_hash(hash_strings[i]), + dict_str_hash(hash_strings[j])); +} + void test_primes_geq() { CU_ASSERT_TRUE(is_prime(2)); diff --git a/util.h b/util.h index b60dccc8..392239a3 100644 --- a/util.h +++ b/util.h @@ -2,22 +2,27 @@ #define UTIL_H__ #include +#include +#include +#include -void shuffle(char **p, size_t size); -bool is_prime(unsigned n); -bool next_permutation(unsigned a[], unsigned n); +static inline void shuffle(char **p, size_t size); +static inline bool is_prime(unsigned n); +static inline bool next_permutation(unsigned a[], unsigned n); -void shuffle(char **p, size_t size) +static inline void shuffle(char **p, size_t size) { + if (size < 2) + return; for (size_t i = 0; i < size - 1; i++) { size_t n = (size_t) rand() % (size - i); char *t = p[i+n]; p[i+n] = p[i]; p[i] = t; } } -bool is_prime(unsigned n) +static inline bool is_prime(unsigned n) { - if (n <= 0) + if (n < 2) return false; if (n <= 3) return true; @@ -39,7 +44,7 @@ bool is_prime(unsigned n) } -bool next_permutation(unsigned a[], unsigned n) +static inline bool next_permutation(unsigned a[], unsigned n) { if (n <= 1) return false;