[Toybox] [CLEANUP][watch.c]

[enh]

(less importantly, i wondered if the \e escapes in interestingtimes.c are
your plan for the future and will replace \033, or should be \e for
consistency. personally, i find \e less unreadable -- i even prefer \x1b to
\033 -- but \e is not actually in the standard.)

On Mon, Aug 20, 2018 at 12:39 PM enh <enh at google.com> wrote:

>
>
> 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/abbdcac3/attachment.htm>