On Sun, Aug 19, 2018 at 2:44 PM Rob Landley <[email protected]> wrote:
> On 08/16/2018 08:37 PM, haroon maqsood wrote: > > Hi PFA, > > my attempt at cleanup of watch.c in pending. > > Thanks > > Haroon > > A little more description of what you did would be nice. Let's see... > requiring > -n to be at least one second makes sense, although debian's can do "-n > 0.1". > (That gets into the xparsetime() granularity issue from > https://landley.net/notes-2018.html#05-01-2018 , an infrastructure todo > item of > mine.) > > time_t is a long (except on x32 where it's a long long) so the { 0 } > initializing it is weird, and why initialize it at all because it's only > ever > used in a printf() two lines after it's written to by time(&t)? > > You're checking if toys.optargs is null which can't happen, you're adding > code > to special case "" as an argument, and you combined the two loops into one > which > is clever but doesn't actually save you anything (an empty loop statement > costs > about as much as an if() statement, and now you have an extra test in the > loop > each time and a result that's harder to read.) > > You moved the declaration of "width" into the middle of code instead of > grouped > at the top of the block (yes compilers play games with stack scope but I > like > variable declarations to be easy to _find_, terminal_size() never sets a > value > to zero (there's a comment in that function about it, it leaves the default > value, you have 80 in there twice?), speaking of terminal_size, is there a reason it doesn't call terminal_probesize if the ioctl fails? it seems like terminal_probesize is only used in top, in response to a signal (presumably for SIGWINCH). whereas terminal_size is used in all kinds of places, but never falling back to probing. even in hexedit.c, which does call scan_key (which is where the other half of terminal_probesize currently lives). would it make more sense to have the scan_key "scratch" buffer be a hidden buffer in lib? > why are you creating a temporary variable > for ctime()'s return value when it's only used once and previously the > function > was called in the printf() arguments... > > Ah, I see. You're freeing tstr at the end, but ctime() returns a static > buffer > so freeing it's an error. And if you run with -t it will never have > assigned > anything to tstr() and you'll free an uninitialized variable. > > Why are you calling xmprintf() and then immediately calling printf on the > result? Why not just call printf? And you don't free the memory xmprintf() > returned so you're leaking it each time through the loop (freeing one copy > at > the end, again an uninitialized variable if run with -t). > > I have the start of a cleanup of watch.c in my tree already, I think I'll > just > finish that one up instead. > > Thanks, > > Rob > _______________________________________________ > Toybox mailing list > [email protected] > http://lists.landley.net/listinfo.cgi/toybox-landley.net >
_______________________________________________ Toybox mailing list [email protected] http://lists.landley.net/listinfo.cgi/toybox-landley.net
