[Toybox] [PATCH] chvt: remove old workarounds.

[enh]

On Sat, Sep 25, 2021 at 9:30 AM Rob Landley <rob at landley.net> wrote:

> On 9/24/21 7:10 PM, enh via Toybox wrote:
> > We don't need the fd=fd hack for GCC 3.4 in 2021,
>
> Yeah, but we still needed it in 2018.
>

do you have a similar 7 year rule for the toolchain used to build toybox,
on the assumption that embedded folks especially are likely to be on gcc
2.95^W^Wold compilers?


> *cracks knuckles* The power of grep '[ ,]\([a-zA-Z0-9_][a-zA-Z0-9_]*\) *=
> *\1'
> *.c lib/*.c toys/*/*.c compels you!
>
> Noise, noise, noise...
>
> toys/net/sntp.c:  unsigned long long *pktime = (void *)toybuf, now, then,
> before
> = before;
> toys/other/chvt.c:  int vtnum, fd = fd;
> toys/other/switch_root.c:  int console = console; // gcc's "may be used"
> warnings are broken.
> toys/other/watch.c:  unsigned width, height, i, cmdlen, len, xx = xx, yy =
> yy,
> active = active;
> toys/posix/uudecode.c:  int ofd, idx = 0, m = m, n;
>
> I could definitely clean that regex up more...
>

yeah, the trouble with this self-assignment pattern is that although it
shuts up that compiler, it annoys static analysis tools and makes it harder
to find the interesting stuff (like the sscanf off-by-one) amongst the
noise.

i know your argument about not wanting the compiler to emit "provably
useless" assignments, but since Android builds everything with stack
variables allocated to 0 anyway [and lets the backend sort out which ones
_it_ can prove aren't needed], this pattern isn't helpful to us. and it
confuses _human_ readers of the code. i get quite a lot of "i found a bug
in toybox" feedback from self-assignment.

(not right now, but at some point i'd like to turn on the self-assignment
warning for Android. it's more usually a mistake than rob landley arguing
that the code that follows is correct, just not obviously so :-) [i forgot
to include in my commit message that i also removed the redundant !*cc
check, which seems like the person who originally wrote the code didn't
quite trust that fd would definitely be -1 by this point either! i've
certainly been accused of writing overly "clever" code before, but i do at
least try not to check in code that i don't at least understand myself :-)
as someone i used to work with used to say: "i don't know much about future
me, but it's a safe bet i won't be getting any cleverer with time".])


> > and <linux/vt.h> has
> > given names to the two magic numbers since at least Linux 2.4.0...
>
> Heh, back when I was trying not to include linux/*.h at all, before I can
> up
> with the "command can include it, headers shouldn't" policy.
>

yeah, speaking of style, i felt like 2021 rob would have `int
vt=atoi(*toys.optargs), fd;` rather than the separate line for the atoi(),
but wasn't sure so i left that even though i touched both lines. perhaps
more interestingly, i wondered whether atoi() is quite right there: should
`chvt foo` change to vt 0? (but since Android didn't use this, i had no
opinion and ignored it. i'm literally only here because it was nearby the
static analyser's complaint about scanf, and i thought "ideally i'd never
have to see this again" :-) )


> (Back in 2008 Steve Jobs wasn't dead yet and smartphones were brand new, so
> "mac/darwin might take over the world or at least meat shield us from the
> windopoly" was still a viable concept. lib/portability.h showed up in jan
> 2007
> but sticking linux/ headers in there didn't really help, and portability.c
> didn't show up until 2012...)
>

for all i complain about the pain of building for macOS (and especially the
lack of convenient cross-compilation), i was genuinely surprised they
didn't have POSIX timers. especially since they did finally add
CLOCK_MONOTONIC for clock_gettime(). maybe in 2031 we can delete this...


> I do note that:
>
> - if (-1 != (fd = open(*cc, O_RDWR))) break;
> + if ((fd = open(*cc, O_RDWR)) != -1) break;
>
> Was alas good advice that never quite soaked in. (If you typo x == 1 as x
> = 1 it
> compiles. If you typo 1 = x it does not. But for constructs like this, "do
> the
> thing, then error check the thing" makes more sense to human
> programmers...)
>

this case is fine even in non-yoda style :-)

cc     x.c   -o x
x.c: In function ‘main’:
x.c:7:34: error: lvalue required as left operand of assignment
    7 |   if ((fd = open("foo", O_RDWR)) = -1) return 1;
      |                                  ^


> 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/20210925/98854bc0/attachment.htm>