> Index: subversion/include/svn_cache.h
> ===================================================================
> +/**
> + * Fetches a value indexed by @a key from @a cache into @a *value,
> + * setting @a found to TRUE iff it is in the cache. The value is
> + * copied into @a pool using the copy function provided to the cache's
> + * constructor.
> + */
Say that you set found to FALSE if not found; this implies that
you just leave found alone.
> Index: subversion/libsvn_subr/cache.c
> ===================================================================
> +struct svn_cache_t {
> + /* Maps from a key (of size CACHE->KLEN) to a struct cache_entry. */
> + apr_hash_t *hash;
> + apr_ssize_t klen;
Maybe it doesn't matter, but why can keys be only one size?
apr_hash_get takes the key size, rather than storing it in the
apr_hash_t itself. This way you can use a string/length in
addition to a null-terminated string.
> +struct cache_entry {
> + const void *key;
> + void *value;
These should both be const. It's the object pointed to you're
protecting, not the pointer, which you can and do write to.
> +/* Uses CACHE->dup_func to copy VALUE into *VALUE_P inside POOL, or
> + just sets *VALUE_P to NULL if VALUE is NULL. */
> +static svn_error_t *
> +duplicate_value(void **value_p,
> + svn_cache_t *cache,
> + void *value,
> + apr_pool_t *pool)
> +{
> + if (value)
> + SVN_ERR((cache->dup_func)(value_p, value, pool));
> + else
> + *value_p = NULL;
> + return SVN_NO_ERROR;
> +}
You take care to handle NULL values here, but apr_hash can't
handle them anyway, right? It uses a NULL value to remove a key.
> +/* Copies KEY into *KEY_P inside POOL, using CACHE->KLEN to figure out
> + * how. */
> +static const void *
> +duplicate_key(svn_cache_t *cache,
> + const void *key,
> + apr_pool_t *pool)
You don't copy to key_p, but return the copy.
> +svn_error_t *
> +svn_cache_get(void **value_p,
> + svn_boolean_t *found,
> + svn_cache_t *cache,
> + const void *key,
> + apr_pool_t *pool)
> +{
> + void *entry_void;
What's the point of this? entry = apr_hash_get(...) should work.
> +static void
> +erase_page(svn_cache_t *cache,
> + struct cache_page *page)
> +{
> + remove_page_from_list(page);
> + struct cache_entry *e;
Statement before declaration.
> +svn_error_t *
> +svn_cache_set(svn_cache_t *cache,
> + const void *key,
> + void *value,
> + apr_pool_t *pool)
> +{
> + void *existing_entry;
> + svn_error_t *err = SVN_NO_ERROR;
> +
> + SVN_ERR(lock_cache(cache));
> +
> + existing_entry = apr_hash_get(cache->hash, key, cache->klen);
> +
> + /* Is it already here, but we can do the one-item-per-page
> + * optimization? */
> + if (existing_entry && cache->items_per_page == 1)
> + {
> + /* Special case! ENTRY is the *only* entry on this page, so
> + * why not wipe it (so as not to leak the previous value).
> + */
> + struct cache_entry *entry = existing_entry;
> + struct cache_page *page = entry->page;
> +
> + /* This can't be the partial page, items_per_page == NULL
"items_per_page == 1"
Reviewing the changes that use the cache next...
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-03-25 22:57:58 CET