On 03/10/2011 08:02 AM, Steven Simpson wrote: > 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...
'stg' can help edit existing patch series. To combine, I often save the individual patches with format-patch, reset the tree to before the patches were applied, and manually apply them with patch -p1 < ... and then git commit the resulting thing. One suggestion: Be sure to save the patches clear of the tree before you go start messing with git reset, though normally you can recover from pretty much anything you do in git... > >> 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. There may be places using 0, but I vastly prefer NULL for pointers, and would be happy to accept any patches that removed usage of '0' for pointers. > >> 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. Those are fine with me. > > Btw, disable_profile=True also raised an error in olsr4.tgt, so I had to > temporarily remove profile/0.1 to get beyond it. Probably best to disable OLSR instead. I don't think anyone actually uses it, so I didn't bother to fix the disable_profile problems in it. Thanks, Ben -- Ben Greear <[email protected]> Candela Technologies Inc http://www.candelatech.com _______________________________________________ Xorp-hackers mailing list [email protected] http://mailman.ICSI.Berkeley.EDU/mailman/listinfo/xorp-hackers
