Re: [PATCH] raw-FD related AsyncCall classes

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.
Received on Wed Nov 30 2011 - 00:22:31 MST