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

Branko ÄŒibej wrote:
> On 16.04.2013 15:29, C. Michael Pilato wrote:
>> On 04/16/2013 08:33 AM, julianfoad_at_apache.org wrote:
>>> Introduce a typedef 'svn_fs_freeze_func_t', for source code regularity and
>>> symmetry with 'svn_repos_freeze_func_t'.  It might also be useful for the
>>> SWIG bindings.
>>
>> When callback signatures are this generic, is there any good reason to
>> proliferate their unique definition?  Users don't really get any benefit
>> by their having a unique type name that I can surmise.  And every time we
>> introduce a new callback type, the SWIG bindings must be tinkered with to
>> support it.

To clarify about SWIG, I understand from Mike's IRC comments [1], 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:
>>
>> * eliminate svn_repos_freeze_func_t, and simply make svn_repos_freeze()
>> also accept this new svn_fs_freeze_func_t, or
>>
>> * eliminate both of those callback types, and introduce (in svn_types.h) an
>> uber-generic callback that accepts a void * baton and a scratch_pool and is
>> used for both svn_repos_freeze() and svn_fs_freeze(), plus any future such
>> generic callback scenarios
>>
>> ?
>
> What's the point of that?
>
> I think it's better to have explicit typedefs for every specific kind of
> callback, even if the actual type signatures are the same. It adds no
> burden to the callback implementor (after all, they just have to define
> a callback function in any case), and it does add a teensy bit of extra
> self-documenting goodness to the signatures of the functions that accept
> callbacks.

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,
                                                       apr_pool_t *result_pool,
                                                       apr_pool_t *scratch_pool);

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.

- Julian

[1] IRC log, <http://colabti.org/irclogger/irclogger_log/svn-dev?date=2013-04-15#l217>.
Received on 2013-04-16 19:31:57 CEST