Re: [RFC] remove JobCallback macro

On 07/18/2014 08:20 PM, Amos Jeffries wrote:

> I have once again been foiled by brokenness in the JobCallback macro
> provided apparently for convenience generating Job based AsyncCalls.

Here is the current JobCallback definition, for reference:

> /// Convenience macro to create a Dialer-based job callback
> #define JobCallback(dbgSection, dbgLevel, Dialer, job, method) \
> asyncCall((dbgSection), (dbgLevel), #method, \
> Dialer(CbcPointer<Dialer::DestClass>(job), &method))
>

> This time I seem to have found the problem though. It stems from
> JobCalback requiring a Dialer+Job+method - with zero parameter objects
> accepted. If one attempts to use it with UnaryMemFunT<> the compile
> breaks with some fairly cryptic errors.

I do not think cryptic compiler messages during macro misuse qualify as
macro brokenness. You could argue that macro documentation is broken
(not detailed enough).

Yes, JobCalback requires a Dialer, a job, and a method -- those are the
macro parameters so that part should be clear. What is less clear and
currently has to be deciphered from the macro definition or usage
examples is that the Dialer parameter is a name of a class (with certain
requirements), the job parameter is a job object pointer, and method is
a full Job::method name.

I am guessing that you do not like that the macro only supports Dialers
that have a two-argument constructor (the last line of the macro
definition creates a Dialer object using that constructor). That is the
price for not having to write

   MyDialer myDialer(CbcPointer<MyDialer::DestClass>(job), &method);

manually before creating a simple callback that does not require
additional Dialer arguments.

> The resolution to this is to use the asyncCall() template function
> directly

Yes, that is one solution. We could also create JobCalback1 and other
convenience macros (that accept more parameters) if desired.

> which JobCallback was supposedly making easier.

And JobCallback does make it easier, but only for the supported Dialers.
It is not broken, just limited to certain common Dialers.

> However, the
> asyncCall is not particularly hard to use in the first place and has
> lots of code examples to look at now.

IMO, the biggest problem with asyncCall() is that it requires to
manually type the method description string. That leads to copy-paste
bugs we have had to correct already.

> Give that JobCallback() is only usable (and used) by the
> NullaryMemFunT<>

or any other Dialer that does not require additional constructor
arguments and provides Dialer::DestClass.

> and where the CommCalls parameter hack/workaround means
> the parameters variable need not be provided to the dialer constructor.

That is unrelated, I think. JobCallback supports any job Dialer that
does not require additional parameters, not just Comm callbacks. Comm
callback dialers are just the most common example of such dialers.

> I am believing that this particualar macro is providing almost no
> benefit outside of CommCalls, so we should do one of the following:

Arguably, the only _harm_ it provides is difficult-to-decipher compiler
errors when the developer attempts to use the macro with an unsupported
dialer. That can probably be addressed by explicitly documenting which
Dialers are supported.

> a) move it to the CommCalls.h header and rename it specific to that API
> to avoid others hitting the same confusion in future.

The API is not specific to Comm calls AFAICT.

> b) remove JobCallback macro completely in favour of asyncCall.

You are already welcome to use asyncCall if you prefer that API.

> c) replace existing JobCallback with overloaded inline template
> equivalents to asyncCall() so that UnaryMemFunT parameter can be provided.

A function is not possible, unfortunately, because the "#method" part of
the JobCallback definition requires a macro.

In (c) spirit, we can provide additional macros to support more Dialers
or a different macro that supports Dialer objects instead of the Dialer
class name (adding one more line to the caller).

> I favour b or c.
> * b makes the code cleaner and more consistent regarding Call creation.

Not cleaner (because it adds code duplication), just more uniform.

> * c offers the chance to hide lots of Unary/Nullary-MemFunT definitions
> inside the inline function via overloading.

Not possible as discussed above.

Hitting a compiler error when supplying a wrong Dialer type does not
sound like a problem big enough to warrant changing the old code,
especially since we have quite a few JobCallback users throughout the
code (more than asyncCall users!).

> $ fgrep -RI JobCallback src | wc -l
> 54
> $ fgrep -RI asyncCall\( src | wc -l
> 43

For the new code, I would not object to either deprecating the old macro
in favor of direct asyncCall() use OR documenting Dialer expectations
and adding more macros to support additional number of Dialer arguments.
The latter would be my preference for the reasons discussed below.

We currently do not have a good callback API at all. We have a decent
call API, but no good callback API where the call object creator is not
the one setting the actual call parameters. That is why we are forced to
use rather ugly code to set call parameters for Comm and PeerConnect
callbacks, to name just two examples.

Providing a decent callback API probably requires changing the Dialer
API so I recommend shying away from solutions that require more explicit
Dialer [creation] code.

HTH,

Alex.
Received on Sun Jul 20 2014 - 17:44:56 MDT