On Tue, Mar 25, 2008 at 2:57 PM, Eric Gillespie <epg_at_google.com> wrote:
> > 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.
Right.
> > 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.
Can you give me an example of any hash in the Subversion codebase that
doesn't always use the same klen argument for every call to
apr_hash_get and apr_hash_set?
> > +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.
That makes sense. I think I did that because of how apr_hash_this has
"const void **key" and "void **val" (ie, in my head somehow I was
thinking that non-const values made sense in APR), but this isn't even
relevant at all.
> > +/* 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.
Right, but the values in the hash are cache_entry structs, whose
'value' member can be NULL. I've often found myself frustrated that
APR hashes can't store NULL values; I didn't see any reason to
continue that in this API.
> > +/* 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.
Oops.
> > +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.
Er. No point.
> > +static void
> > +erase_page(svn_cache_t *cache,
> > + struct cache_page *page)
> > +{
> > + remove_page_from_list(page);
> > + struct cache_entry *e;
>
> Statement before declaration.
Gah! Thanks.
> > +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"
Whoops.
--dave
--
David Glasser | glasser@davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
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 23:29:38 CET