On 10/06/2009 05:44 AM, Bruce Simpson wrote: > Ben Greear wrote: > Comments: > * It should be possible to turn off the millisecond logging if desired. > Whilst it's certainly a useful feature to have when debugging time > contingent code, it does add clutter to the output. > * Perhaps putting it under the other debug knobs in SConstruct would be > a good idea?
Will add an #ifdef that could be twiddled in scons. > * %llu is not a portable format specifier, and 'unsigned long long' is > not a portable type, please don't use them in portable code. Ok, I can use uint64_t, but what do you use instead of %llu to print it? > * Perhaps the code which prints the timeval is a candidate for a > function like xlog_localtime2string_short() ? > > * xlog_localtime2string_short() is still defined in xlog.c; so why > comment out its prototype, are you getting warnings from the compiler? It was all commented out...I removed entirely now. > * A XorpTimer of 0 is a possible candidate for a XorpTask. I can't > really delve further into that change at the moment, though. > * Yes, it may be useful to constify the string arguments in those > callback functions, but this change considered low priority. > * Please avoid introducing unnecessary whitespace changes in diffs. > > > Can you please raise a Trac item for these suggested improvements? > I probably won't have time to look at the Router Manager in detail for > at least 4 weeks. > > Sorry for the bureaucracy... I appreciate you're doing what you can in > the here and now to improve the code, however, it makes reviewing > patches and applying them that much easier, and we do need to keep the > code alignment and type clean, etc. I'll let these changes perk in my tree..plz let me know when you're ready to work on it (no hurry) and I'll make diffs against upstream and we can quickly resolve any issues and commit the code. In the meantime, these reviews are appreciated and will help make the eventual merge easier I think. 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
