cache: redesign

cache uses an invasive design now, a la list.h and uthash. This get rid
of the ugly integer to pointer cache, and gives us freedom of what we
can put into the cache.

Signed-off-by: Yuxuan Shui <yshuiv7@gmail.com>

src/atom.c

@@ -2,14 +2,19 @@
 #include <xcb/xcb.h>
 
 #include "atom.h"
+#include "cache.h"
 #include "common.h"
-#include "utils.h"
+#include "compiler.h"
 #include "log.h"
+#include "utils.h"
 
-static inline void *atom_getter(void *ud, const char *atom_name, int *err) {
+static inline int
-	xcb_connection_t *c = ud;
+atom_getter(struct cache *cache, const char *atom_name, struct cache_handle **value) {
+	struct atom *atoms = container_of(cache, struct atom, c);
 	xcb_intern_atom_reply_t *reply = xcb_intern_atom_reply(
-	    c, xcb_intern_atom(c, 0, to_u16_checked(strlen(atom_name)), atom_name), NULL);
+	    atoms->conn,
+	    xcb_intern_atom(atoms->conn, 0, to_u16_checked(strlen(atom_name)), atom_name),
+	    NULL);
 
 	xcb_atom_t atom = XCB_NONE;
 	if (reply) {
@@ -18,9 +23,19 @@ static inline void *atom_getter(void *ud, const char *atom_name, int *err) {
 		free(reply);
 	} else {
 		log_error("Failed to intern atoms");
-		*err = 1;
+		return -1;
 	}
-	return (void *)(intptr_t)atom;
+
+	struct atom_entry *entry = ccalloc(1, struct atom_entry);
+	entry->atom = atom;
+	*value = &entry->entry;
+	return 0;
+}
+
+static inline void
+atom_entry_free(struct cache *cache attr_unused, struct cache_handle *handle) {
+	struct atom_entry *entry = cache_entry(handle, struct atom_entry, entry);
+	free(entry);
 }
 
 /**
@@ -28,11 +43,16 @@ static inline void *atom_getter(void *ud, const char *atom_name, int *err) {
  */
 struct atom *init_atoms(xcb_connection_t *c) {
 	auto atoms = ccalloc(1, struct atom);
-	atoms->c = new_cache((void *)c, atom_getter, NULL);
+	atoms->conn = c;
-#define ATOM_GET(x)                                                                      \
+	cache_init(&atoms->c, atom_getter);
-	atoms->a##x = (xcb_atom_t)(intptr_t)cache_get_or_fetch(atoms->c, #x, NULL)
+#define ATOM_GET(x) atoms->a##x = get_atom(atoms, #x)
 	LIST_APPLY(ATOM_GET, SEP_COLON, ATOM_LIST1);
 	LIST_APPLY(ATOM_GET, SEP_COLON, ATOM_LIST2);
 #undef ATOM_GET
 	return atoms;
 }
+
+void destroy_atoms(struct atom *a) {
+	cache_invalidate_all(&a->c, atom_entry_free);
+	free(a);
+}

src/atom.h

@@ -4,6 +4,7 @@
 #include <xcb/xcb.h>
 
 #include "cache.h"
+#include "log.h"
 #include "meta.h"
 
 // clang-format off
@@ -54,22 +55,33 @@
 #define ATOM_DEF(x) xcb_atom_t a##x
 
 struct atom {
-	struct cache *c;
+	xcb_connection_t *conn;
+	struct cache c;
 	LIST_APPLY(ATOM_DEF, SEP_COLON, ATOM_LIST1);
 	LIST_APPLY(ATOM_DEF, SEP_COLON, ATOM_LIST2);
 };
 
+struct atom_entry {
+	struct cache_handle entry;
+	xcb_atom_t atom;
+};
+
+/// Create a new atom object with a xcb connection, note that this atom object will hold a
+/// reference to the connection, so the caller must keep the connection alive until the
+/// atom object is destroyed.
 struct atom *init_atoms(xcb_connection_t *);
 
 static inline xcb_atom_t get_atom(struct atom *a, const char *key) {
-	return (xcb_atom_t)(intptr_t)cache_get_or_fetch(a->c, key, NULL);
+	struct cache_handle *entry = NULL;
+	if (cache_get_or_fetch(&a->c, key, &entry) < 0) {
+		log_error("Failed to get atom %s", key);
+		return XCB_NONE;
+	}
+	return cache_entry(entry, struct atom_entry, entry)->atom;
 }
 
 static inline xcb_atom_t get_atom_cached(struct atom *a, const char *key) {
-	return (xcb_atom_t)(intptr_t)cache_get(a->c, key);
+	return cache_entry(cache_get(&a->c, key), struct atom_entry, entry)->atom;
 }
 
-static inline void destroy_atoms(struct atom *a) {
+void destroy_atoms(struct atom *a);
-	cache_free(a->c);
-	free(a);
-}

src/cache.c

@@ -1,94 +1,56 @@
 #include <uthash.h>
 
-#include "compiler.h"
-#include "utils.h"
 #include "cache.h"
 
-struct cache_entry {
+struct cache_handle *cache_get(struct cache *c, const char *key) {
-	char *key;
+	struct cache_handle *e;
-	void *value;
-	UT_hash_handle hh;
-};
-
-struct cache {
-	cache_getter_t getter;
-	cache_free_t free;
-	void *user_data;
-	struct cache_entry *entries;
-};
-
-static inline struct cache_entry *cache_get_entry(struct cache *c, const char *key) {
-	struct cache_entry *e;
 	HASH_FIND_STR(c->entries, key, e);
 	return e;
 }
 
-void *cache_get(struct cache *c, const char *key) {
+int cache_get_or_fetch(struct cache *c, const char *key, struct cache_handle **value) {
-	struct cache_entry *e = cache_get_entry(c, key);
+	*value = cache_get(c, key);
-	return e ? e->value : NULL;
+	if (*value) {
+		return 0;
+	}
+
+	int err = c->getter(c, key, value);
+	assert(err <= 0);
+	if (err < 0) {
+		return err;
+	}
+	(*value)->key = strdup(key);
+
+	HASH_ADD_STR(c->entries, key, *value);
+	return 1;
 }
 
-void *cache_get_or_fetch(struct cache *c, const char *key, int *err) {
+static inline void
-	struct cache_entry *e = cache_get_entry(c, key);
+cache_invalidate_impl(struct cache *c, struct cache_handle *e, cache_free_t free_fn) {
-	if (e) {
-		return e->value;
-	}
-
-	int tmperr;
-	if (!err) {
-		err = &tmperr;
-	}
-
-	*err = 0;
-	e = ccalloc(1, struct cache_entry);
-	e->key = strdup(key);
-	e->value = c->getter(c->user_data, key, err);
-	if (*err) {
-		free(e->key);
-		free(e);
-		return NULL;
-	}
-
-	HASH_ADD_STR(c->entries, key, e);
-	return e->value;
-}
-
-static inline void cache_invalidate_impl(struct cache *c, struct cache_entry *e) {
-	if (c->free) {
-		c->free(c->user_data, e->value);
-	}
 	free(e->key);
 	HASH_DEL(c->entries, e);
-	free(e);
+	if (free_fn) {
+		free_fn(c, e);
+	}
 }
 
-void cache_invalidate(struct cache *c, const char *key) {
+void cache_invalidate(struct cache *c, const char *key, cache_free_t free_fn) {
-	struct cache_entry *e;
+	struct cache_handle *e;
 	HASH_FIND_STR(c->entries, key, e);
 
 	if (e) {
-		cache_invalidate_impl(c, e);
+		cache_invalidate_impl(c, e, free_fn);
 	}
 }
 
-void cache_invalidate_all(struct cache *c) {
+void cache_invalidate_all(struct cache *c, cache_free_t free_fn) {
-	struct cache_entry *e, *tmpe;
+	struct cache_handle *e, *tmpe;
 	HASH_ITER(hh, c->entries, e, tmpe) {
-		cache_invalidate_impl(c, e);
+		cache_invalidate_impl(c, e, free_fn);
 	}
 }
 
-void *cache_free(struct cache *c) {
+void cache_init(struct cache *cache, cache_getter_t getter) {
-	void *ret = c->user_data;
+	cache->getter = getter;
-	cache_invalidate_all(c);
+	cache->entries = NULL;
-	free(c);
-	return ret;
-}
-
-struct cache *new_cache(void *ud, cache_getter_t getter, cache_free_t f) {
-	auto c = ccalloc(1, struct cache);
-	c->user_data = ud;
-	c->getter = getter;
-	c->free = f;
-	return c;
 }

src/cache.h

@@ -1,30 +1,46 @@
 #pragma once
 
+#include <uthash.h>
+#include "utils.h"
+
+#define cache_entry(ptr, type, member) container_of(ptr, type, member)
+
 struct cache;
+struct cache_handle;
 
-typedef void *(*cache_getter_t)(void *user_data, const char *key, int *err);
+/// User-provided function to fetch a value for the cache, when it's not present.
-typedef void (*cache_free_t)(void *user_data, void *data);
+/// Should return 0 if the value is fetched successfully, and a negative number if the
+/// value cannot be fetched. Getter doesn't need to initialize fields of `struct
+/// cache_handle`.
+typedef int (*cache_getter_t)(struct cache *, const char *key, struct cache_handle **value);
+typedef void (*cache_free_t)(struct cache *, struct cache_handle *value);
 
-/// Create a cache with `getter`, and a free function `f` which is used to free the cache
+struct cache {
-/// value when they are invalidated.
+	cache_getter_t getter;
-///
+	struct cache_handle *entries;
-/// `user_data` will be passed to `getter` and `f` when they are called.
+};
-struct cache *new_cache(void *user_data, cache_getter_t getter, cache_free_t f);
+
+struct cache_handle {
+	char *key;
+	UT_hash_handle hh;
+};
+
+/// Initialize a cache with `getter`
+void cache_init(struct cache *cache, cache_getter_t getter);
 
 /// Get a value from the cache. If the value doesn't present in the cache yet, the
 /// getter will be called, and the returned value will be stored into the cache.
-void *cache_get_or_fetch(struct cache *, const char *key, int *err);
+/// Returns 0 if the value is already present in the cache, 1 if the value is fetched
+/// successfully, and a negative number if the value cannot be fetched.
+int cache_get_or_fetch(struct cache *, const char *key, struct cache_handle **value);
 
 /// Get a value from the cache. If the value doesn't present in the cache, NULL will be
 /// returned.
-void *cache_get(struct cache *, const char *key);
+struct cache_handle *cache_get(struct cache *, const char *key);
 
 /// Invalidate a value in the cache.
-void cache_invalidate(struct cache *, const char *key);
+void cache_invalidate(struct cache *, const char *key, cache_free_t free_fn);
 
-/// Invalidate all values in the cache.
+/// Invalidate all values in the cache. After this call, `struct cache` holds no allocated
-void cache_invalidate_all(struct cache *);
+/// memory, and can be discarded.
-
+void cache_invalidate_all(struct cache *, cache_free_t free_fn);
-/// Invalidate all values in the cache and free it. Returns the user data passed to
-/// `new_cache`
-void *cache_free(struct cache *);

src/list.h

@@ -2,18 +2,7 @@
 #include <stdbool.h>
 #include <stddef.h>
 
-/**
+#include "utils.h"
- * container_of - cast a member of a structure out to the containing structure
- * @ptr:	the pointer to the member.
- * @type:	the type of the container struct this is embedded in.
- * @member:	the name of the member within the struct.
- *
- */
-#define container_of(ptr, type, member)                                                  \
-	({                                                                               \
-		const __typeof__(((type *)0)->member) *__mptr = (ptr);                   \
-		(type *)((char *)__mptr - offsetof(type, member));                       \
-	})
 
 struct list_node {
 	struct list_node *next, *prev;

src/utils.h

@@ -117,6 +117,20 @@ safe_isnan(double a) {
 		ASSERT_IN_RANGE(__to_tmp, 0, max);                                       \
 		(uint32_t) __to_tmp;                                                     \
 	})
+
+/**
+ * container_of - cast a member of a structure out to the containing structure
+ * @ptr:	the pointer to the member.
+ * @type:	the type of the container struct this is embedded in.
+ * @member:	the name of the member within the struct.
+ *
+ */
+#define container_of(ptr, type, member)                                                  \
+	({                                                                               \
+		const __typeof__(((type *)0)->member) *__mptr = (ptr);                   \
+		(type *)((char *)__mptr - offsetof(type, member));                       \
+	})
+
 /**
  * Normalize an int value to a specific range.
  *