On 11/24/2011 04:04 AM, Amos Jeffries wrote:
> Updated patch for review.
> + UnaryCbdataDialer(Handler *aHandler, CbcPointer<Argument1> aArg) :
The second parameter can probably be adjusted to "const
CbcPointer<Argument1>&" to minimize cbdata-refcounting overheads, but
see below.
> +// helper function to simplify Call creation.
s/Call/UnaryCbdataDialer/ or just s/Call/Dialer/
> +cbdataDialer(typename UnaryCbdataDialer<Argument1>::Handler *handler,
> Argument1 *arg1)
I am concerned that cbdataDialer() arg1 type does not match that of
UnaryCbdataDialer constructor. This is atypical for such helper
functions. You may want to change the constructor to use a raw pointer
as well.
> +#if 0
> +// dialer to run cbdata object members as Async Calls
> +// to ease the transition of these cbdata objects to full Jobs
> +template<class T>
> +class UnaryCbdataDialer : public CallDialer
Please do not forget to remove during commit.
> + int fd; /// FD which the call was about. Set by the async call creator.
Missing "<" after "///".
> + virtual bool canDial(AsyncCall &call) { return arg1.valid(); }
> + void dial(AsyncCall &call) { handler(arg1.get()); }
I do not think canDial() is virtual. AsyncCallT<Dialer> knows its dialer
so it does not need canDial() to be virtual to work. There is a TODO to
make canDial() and dial() virtual but it is blocked by
CommCbFunPtrCallT doing things differently.
I did not find any serious bugs in this patch.
Thank you,
Alex.
