Philip Martin wrote:
> "D.J. Heap" <dj@shadyvale.net> writes:
[snip]
>> if (baton->notify_func)
>> {
>> struct svn_wc_notify_struct_t n; // or put this in a pool?
>
>
> Don't use a pool if you can use the stack.
Ok. I wasn't sure about the expected lifetime -- but after looking at
the code more closely, it's obvious it will be fine on the stack. The
callback cannot expect the parameters to be valid after the call because
subpools containing some of the parameters are cleared on each teration.
>
>
>> memset(&n, 0, sizeof(n));
>
>
> I dislike like unnecessary initialisation, I feel it hides bugs.
Ok.
[snip]
>
>
>>3. Leave well enough alone. Very easy to do. Only real drawback is
>>some pain for clients when new fields are passed in the callbacks.
>
>
> I'm not sure I understand the above.
>
Well, one of the reasons I'm interested in changing this is to help
myself -- client programs may not care (at least immediately) about new
informational fields like the proposed progress indicator stuff. If the
callback uses a struct, then they just ignore the added fields and can
rebuild with no problems. Right now, adding new fields will break all
the clients because the callback signature changes -- not a big deal,
but kind of a pain.
> The notify callback has several "actions", and some of the parameters
> are not relevant for all the actions. This is one of the reasons I
> dislike the memset initialisation, why bother setting a field that is
> not used? Perhaps it would be clearer to use a union of structs, with
> one struct for each action or group of actions, so that it is easy to
> determine which parameters are relevant for each action.
>
I like that idea -- it looks like there are 3 basic categories:
General notifications:
svn_wc_notify_add = 0,
svn_wc_notify_copy,
svn_wc_notify_delete,
svn_wc_notify_restore,
svn_wc_notify_revert,
svn_wc_notify_failed_revert,
svn_wc_notify_resolve,
svn_wc_notify_status,
svn_wc_notify_skip,
Update notifications:
svn_wc_notify_update_delete,
svn_wc_notify_update_add,
svn_wc_notify_update_update,
svn_wc_notify_update_completed,
svn_wc_notify_update_external,
And commit notifications:
svn_wc_notify_commit_modified,
svn_wc_notify_commit_added,
svn_wc_notify_commit_deleted,
svn_wc_notify_commit_replaced,
svn_wc_notify_commit_postfix_txdelta
So it looks like it'd end up with a notify-callback struct that includes
the notify action (and probably node kind?) and 3 unioned structs, the
action determining which union to use. A helper function would set the
relevant fields in the appropriate union based on the notify action. Is
that what you were thinking?
Does this seem like a useful change, or not really?
DJ
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Jul 16 04:24:54 2003