[Xorp-hackers] [PATCH] Supporting asynchronous method implementations

[Ben Greear]

On 03/14/2011 05:14 AM, Steven Simpson wrote:
> Hi Ben,
>
> On 11/03/11 14:59, Ben Greear wrote:
>> This looks a lot better.  There are a few things that bother me,
>> but not sure there is a better way to do it:
>>
>> * I don't like the typedef'd return values that change based on
>> enabling/disabling the async-server.
>
> Yes, it bothered me that it might be hard to follow, but at least it
> doesn't spill out into the process-writer's domain, and it rather neatly
> (I think) avoids maintenance of two /almost/ identical lines or blocks
> in a few cases.
>
>> Maybe we can use a helper method somehow?
>
> Not sure what you mean here, but I fear that anything else would just
> degenerate into awkard #ifdefs and almost duplicate maintenance.  I'm
> open to suggestions, though.
>
>>    I don't mind if enabling async-server adds a small bit
>> of code/size to the binaries, so not everything has to be #ifdef'd
>> out, for instance.
>
> I didn't see many opportunities for keeping things in regardless of the
> sync-async choice, that I haven't taken already.

When I apply the patch, I'll look it over and see if anything comes
to mind.

>> * The Macro that returns one type or another should be all caps
>>   at the least.  It will be confusing either way, but at least all-caps
>>   gives the reader a clue it's a macro instead of a standard function.
>
> Okay - there are three macros:
>
>      * XrlCmdReturnError
>      * XrlCmdOptCallback
>      * XrlDispatcherReturnError
>
> How exactly should they be capitalised?  Will XRL_CMD_RETURN_ERROR do,
> for example?  Or XORP_something?

No need to prepend with XORP I think, so the first one looks fine to me.

>> Have you run 'scons check' with and without this enabled?
>
> I have now.  Diffing isn't terribly helpful - timestamps and process ids
> vary - but I see from a partial scan that most lines have a counterpart
> in each file, with a little re-ordering.  Just grepping for "Tests"
> showed the same successes and failures in both cases - I hope those are
> the most important lines.

If it mostly looks OK, that is good enough for me.  I have some post-processing
of the test results in the buildbot that I'll be sure to check when this
is committed.

That said, I'm going to release 1.8.3 before applying your patch.  1.8.3
should be released in 1-2 weeks.

Thanks,
Ben


-- 
Ben Greear <greearb at candelatech.com>
Candela Technologies Inc  http://www.candelatech.com