
<html>

<head>

<meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1">

<style type="text/css" style="display:none;"> P {margin-top:0;margin-bottom:0;} </style>

</head>

<body dir="ltr">

<div style="font-family: Calibri, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);">

Hi Rob,</div>

<div style="font-family: Calibri, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);">

Thanks a lot for the feedback, </div>

<div style="font-family: Calibri, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);">

Noted, i will be more careful next time .</div>

<div style="font-family: Calibri, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);">

Haroon</div>

<div style="font-family: Calibri, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);">

</div>

<hr style="display:inline-block;width:98%" tabindex="-1">

<div id="divRplyFwdMsg" dir="ltr"><font face="Calibri, sans-serif" style="font-size:11pt" color="#000000"><b>From:</b> Rob Landley <rob@landley.net><br>

<b>Sent:</b> Sunday, August 19, 2018 9:44 PM<br>

<b>To:</b> haroon maqsood; toybox@lists.landley.net<br>

<b>Subject:</b> Re: [Toybox] [CLEANUP][watch.c]</font>

<div> </div>

</div>

<div class="BodyFragment"><font size="2"><span style="font-size:11pt;">

<div class="PlainText">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">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?), 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>

</div>

</span></font></div>

</body>

</html>