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.
