(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 <[email protected]> wrote: > > > On Sun, Aug 19, 2018 at 2:44 PM Rob Landley <[email protected]> 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 >> [email protected] >> http://lists.landley.net/listinfo.cgi/toybox-landley.net >> >
_______________________________________________ Toybox mailing list [email protected] http://lists.landley.net/listinfo.cgi/toybox-landley.net
