[Toybox] [CLEANUP][watch.c]

[haroon maqsood]

Hi Rob,
Thanks a lot for the feedback,
Noted, i will be more careful next time .
Haroon
________________________________
From: Rob Landley <rob at landley.net>
Sent: Sunday, August 19, 2018 9:44 PM
To: haroon maqsood; toybox at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.landley.net/pipermail/toybox-landley.net/attachments/20180820/0948ed81/attachment.html>