Convert Notify/Log/Prompt callbacks to structs

["D.J. Heap" <dj...@shadyvale.net>]

I've taken a quick look at this and, of course, it appears to mostly be 
grunt code converting which I'm willing to do.  However, I have an 
inkling that there could be some stylistic nits that I would rather get 
out of the way before I do much work on it.

Two possible ways to handle the changes to caller code are to:

1.  Change it to build a struct and pass it in place.  Probably 
simplest, but would take the most coding to convert and, IMO, is 
something of a pain to code up new notify callers (gotta build a struct, 
init all fields, etc).  Also has low overhead.  A drawback (IMO) is that 
new fields added to the notify struct may not get set properly in all 
places if careful attention is not paid to all callers when they are 
added -- of course, old caller code may not care to set new fields 
anyway (beyond a zero value).

For example, instead of:

   if (baton->notify_func)
     (*baton->notify_func) (...)

It would be something like:

   if (baton->notify_func)
     {
      struct svn_wc_notify_struct_t n;    // or put this in a pool?
      memset(&n, 0, sizeof(n));
      n.baton = notify_baton;
      n.path = path;
      ...
      (*baton->notify_func) (&n);
     }


2.  Use a wrapper macro or function to build the struct and call the 
callback.  Less typing for me to convert the current caller code, and 
IMO, easier for new callers to use, and when new fields are added the 
old caller code will get an error or warning about not having all the 
parameters.  A macro would introduce less overhead than a wrapper function.

Something like:

   SVN_WC_NOTIFY(baton->notify_func, notify_baton, path, ...)

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.

4. ??

Anyone care to comment?

DJ



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: Convert Notify/Log/Prompt callbacks to structs

["D.J. Heap" <dj...@shadyvale.net>]

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

Re: Convert Notify/Log/Prompt callbacks to structs

[Philip Martin <ph...@codematters.co.uk>]

"D.J. Heap" <dj...@shadyvale.net> writes:

> 1.  Change it to build a struct and pass it in place.  Probably
> simplest, but would take the most coding to convert and, IMO, is
> something of a pain to code up new notify callers (gotta build a
> struct, init all fields, etc).  Also has low overhead.  A drawback
> (IMO) is that new fields added to the notify struct may not get set
> properly in all places if careful attention is not paid to all callers
> when they are added -- of course, old caller code may not care to set
> new fields anyway (beyond a zero value).
>
> For example, instead of:
>
>    if (baton->notify_func)
>      (*baton->notify_func) (...)
>
> It would be something like:
>
>    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.

>       memset(&n, 0, sizeof(n));

I dislike like unnecessary initialisation, I feel it hides bugs.

>       n.baton = notify_baton;
>       n.path = path;
>       ...
>       (*baton->notify_func) (&n);
>      }

Or
    if (baton->notify_func)
      {
         struct svn_wc_notify_struct_t n;
         some_new_function (&n, the, original, args);
         (*baton->notify_func)(&n);
      }

> 2.  Use a wrapper macro or function to build the struct and call the
> callback.  Less typing for me to convert the current caller code, and
> IMO, easier for new callers to use, and when new fields are added the
> old caller code will get an error or warning about not having all the
> parameters.  A macro would introduce less overhead than a wrapper
> function.

The "overhead" of a function call is almost certainly trivial,
particularly in the case when the callback produces some output.

> Something like:
>
>    SVN_WC_NOTIFY(baton->notify_func, notify_baton, path, ...)

I don't like the idea of using a macro here.

> 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.

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.

-- 
Philip Martin

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org