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
