Hi Ben, On 08/03/11 18:07, Ben Greear wrote: > First: It seems at least some of the 3rd patch is changing code from the > first or second patch. This increases the amount of code review needed, > so I'm hoping you can re-work this so that the third patch is merged > into the previous patch(es). You are welcome to have more smaller > patches, > just don't have a later patch do changes to the previous ones if > possible.
I just sent the output of 'git format-patch'; thought it might have generated just one patch, but it didn't and I couldn't find an option to collapse them. I'd rather have sent just one anyway - they're not functional individually, just compilable. I now seem to have managed to produce a single patch with all changes, though that may be redundant in light of the potential #ifdef requirement... > Second: I *think* you are using '0' when you are passing a NULL pointer. > Please use 'NULL' instead so it's obvious it's a pointer type and not an > integer value. I know that they are in practice equivalent on any modern > architecture... Hmm, I suspect I saw 0 being used somewhere already (in FinderMessengerBase::dispatch_xrl?), and assumed it was the local convention. I will use NULL, however, as you request. > Third: Please compare the size of the stripped binaries before and after > your changes. If your changes cause a significant increase in disk space > usage, we may want to allow them to be #ifdef'd out: There are folks who > are trying to use xorp with very small storage systems. You can check > the SConstruct file for examples of how to compile out things (see > 'disable_profile', etc). I think I counted about 2MB increase stripped and using disable_warninglogs=True disable_tracelogs=True disable_fatallogs=True disable_infologs=True disable_assertlogs=True disable_errorlogs=True disable_otherlogs=True disable_profile=True, and 7MB unstripped. I expect it's all the inlined template instantiations for callbacks...? It sounds like a lot, so I'm having a go at an #ifdef'd version. XORP_ENABLE_ASYNC_SERVER is the macro name, and enable_async_server is the scons argument. Let me know if you prefer other names. Btw, disable_profile=True also raised an error in olsr4.tgt, so I had to temporarily remove profile/0.1 to get beyond it. > Fourth: Please add more verbose descriptions to the individual patches. Will do. > And finally: I'd like a: > > Signed-off-by: [you] <your-email> Oh, so that's what that's for! No problem. Cheers, Steven -- _______________________________________________ Xorp-hackers mailing list [email protected] http://mailman.ICSI.Berkeley.EDU/mailman/listinfo/xorp-hackers
