Re: svn commit: r1103414 - /subversion/trunk/subversion/libsvn_fs_fs/caching.c

Stefan Fuhrmann wrote:
> On 16.05.2011 18:47, Julian Foad wrote:
> > Hi Stefan. Please could you give this function a doc string, and
> > init_callbacks() later in the same file. Thanks.
> Took me 4 days to get to this, but now its done.
> See r1125312.

Thanks, Stefan, that's much better now. A few coments...

> > Log:
> > * subversion/libsvn_fs_fs/caching.c
> > (init_callbacks, svn_fs_fs__initialize_caches, init_txn_callbacks,
> > init_txn_callbacks, svn_fs_fs__initialize_txn_caches): add docstrings

In Subversion, when a function is declared in a header file, we put the
doc string there and not at the definition. Please could you move the
relevant ones. Thanks. (I moved some of them recently without telling
you, and maybe these are some of the ones I moved, so my apologies if
that confused you. They may have two doc strings now that need to be
combined.)

> > +/* This function sets / registers the required callbacks for a given
> > + * not transaction-specific CACHE object in FS.

Should say "if CACHE is not NULL" somewhere there.

> > + * All these svn_cache__t instances shall be handled uniformly. That
> > + * applies to the NO_HANDLER flag as well which controls whether the
> > + * error handler will be sets for the cache.

I get what the NO_HANDLER flag does (I can see from the code), and
perhaps we could write that more directly; for example:

  "Unless NO_HANDLER is true, register an error handler that reports
errors as warnings."

Maybe we should rename NO_HANDLER to NO_ERROR_HANDLER or something a bit
more descriptive? Very low priority, as it's just a local function.

I don't get what you mean by "these ... shall be handled uniformly" and
"applies to NO_HANDLER as well".

> > + */
> > static svn_error_t *
> > init_callbacks(svn_cache__t *cache,
> > svn_fs_t *fs,

> +/* This function sets / registers the required callbacks for a given
> + * transaction-specific *CACHE object. In particular, it will ensure
> + * that *CACHE gets reset to NULL upon POOL destruction latest.
> + */
> static void
> init_txn_callbacks(svn_cache__t **cache,
> apr_pool_t *pool)

This one also does nothing if *CACHE is NULL.

- Julian
Received on 2011-05-20 13:09:48 CEST