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