Hi Rob, Thanks a lot for the feedback, Noted, i will be more careful next time . Haroon ________________________________ From: Rob Landley <r...@landley.net> Sent: Sunday, August 19, 2018 9:44 PM To: haroon maqsood; toybox@lists.landley.net Subject: Re: [Toybox] [CLEANUP][watch.c]
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?), 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 Toybox@lists.landley.net http://lists.landley.net/listinfo.cgi/toybox-landley.net