[Toybox] [CLEANUP][watch.c]

[enh]

On Sun, Aug 19, 2018 at 2:44 PM Rob Landley <rob at landley.net> 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
> Toybox at lists.landley.net
> http://lists.landley.net/listinfo.cgi/toybox-landley.net
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.landley.net/pipermail/toybox-landley.net/attachments/20180820/67356aae/attachment.htm>