Re: Convert Notify/Log/Prompt callbacks to structs

"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