Re: svn commit: r1468395 - in /subversion/trunk/subversion: include/svn_fs.h libsvn_fs/fs-loader.c libsvn_fs/fs-loader.h libsvn_fs_base/fs.c libsvn_fs_fs/fs.c
From: Julian Foad <julianfoad_at_btopenworld.com>
Date: Tue, 16 Apr 2013 18:31:24 +0100 (BST)
Branko ńĆibej wrote:
To clarify about SWIG, I understand from Mike's IRC comments , and from Philip's original introduction of the svn_repos_freeze_func_t, that this commit would make it easier to support SWIG bindings than it was before when no typedef was in use at all here.¬† Is that right?¬† But I can see that using just one typedef in both places would be even easier.
>> May I suggest that we either:
Yes.¬† I have no strong opinion either way.¬† That makes sense to me, but so does C-Mike's point about this callback not being specific to the repos layer or the FS layer.¬† In this particular incarnation of the API, we are choosing not to pass any information about the frozen FS's or repositories to the callback, so the callback is completely agnostic of its caller.¬† Because of that, I don't much like the idea of using a typedef named '*_fs_*' in both places and would prefer the second option, using a generic name, if we change it at all.
For interest, I scanned through our header files and found two other callback types defined that are targetted for a particular purpose but are in essence completely generic:
typedef svn_error_t *(*svn_cancel_func_t)(void *cancel_baton);
typedef svn_error_t *(*svn_wc__with_write_lock_func_t)(void *baton,
Every generic callback needs a 'baton', and in Subversion it usually makes sense to return svn_error_t.¬† Every callback that might be called more than once from a hidden context should be passed a scratch pool, so 'cancel_func' is not sufficiently generic; that callback is intended to do only very simple work.¬† The 'result_pool' is made explicit here as a convenience for common usage patterns with _with_write_lock, but (arguably) belongs inside the baton in theory.
The new 'freeze' callback type is, I would argue, the most generic of all.¬† It would be capable of serving both the cancellation and the _with_write_lock purposes.¬† Making a new global typedef for it, and for other generic callbacks in future, would be quite reasonable.
So much for a theoretical perspective.¬† From a practical perspective, since we don't exactly have a proliferation of generic callback usage, I don't see that it makes very much difference what we do here.¬† I am willing to do the grunt work of changing it if anybody wants me to.
 IRC log, <http://colabti.org/irclogger/irclogger_log/svn-dev?date=2013-04-15#l217>.
This is an archived mail posted to the Subversion Dev mailing list.