Re: [PATCH] raw-FD related AsyncCall classes

On 30/11/2011 1:21 p.m., Alex Rousskov wrote:
> 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.

Fixed by doing the constructor change below.

>
>> +// helper function to simplify Call creation.
> s/Call/UnaryCbdataDialer/ or just s/Call/Dialer/
>
Done.

>
>> +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.

Done.

>
>> +#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.

Oops. Dropped now.

>
>> + int fd; /// FD which the call was about. Set by the async call creator.
> Missing "<" after "///".

Done.

>
>> + 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.

grep -R "canDial" src/

It is virtual everywhere else except RockIoState. That might be a bug in
RockStore.

dial() is the one blocked being a virtual by CommCalls.

>
>
> I did not find any serious bugs in this patch.
>
>

Giving it another run around the block for the dialer constructor change
and will commit then if it passes.

Amos
Received on Sun Dec 04 2011 - 01:47:24 MST