[Toybox] [PATCH] toysh: fix -Wuse-after-free
Oliver Webb
aquahobbyist at proton.me
Fri Mar 8 19:22:34 PST 2024
TL;DR: Rant about sh.c's variable names I forgot to include in main email, I
have a patch to start fixing it but it conflicts with other stuff and I have
to re-do it
Reading through sh.c, most of the variable names are 2 letters long (repeating the same letter),
for short functions (<20 lines) this isn't a problem. For example, I don't need a more
descriptive name for the variable "c" in getvar() to understand it because I can look at the uses
of it and infer from context it's about variable types. Longer functions where there are a lot of variables
interacting with each other are a lot harder to keep track of.
For example, what does "cc" do in sh_main? From the looks of it. It's the name
of command/file being processed, but to _know_ that's the case I have to audit
50 lines of code, and check over seeing if that's what it really does. When a name
like "in_source", "inputsrc", or just "inname" would tell you that without having to look
over the code and make sure that's the case.
The problem isn't the length, the problem is that there isn't any convention to it and a lot
of names convey little to no information ("ss" is usually a string, but "what does the string do?"
is a question you need to commonly ask to find out what it's doing). It'd be like if run_lines()
was called "rr()" or "expand_arg" was called "ea()", we name functions just fine (which makes
this code somewhat readable, at least it's general structure)
"ff" is a common name for file descriptors. So working down from sh_main, One could assume that
"TT.ff" must have something to do with file management, right?... No, it's a doubly linked list
of overhead for shell functions, and "struct sh_fcall" is often called "ff" in functions that use
it, we have 2 common names that do completely different things...
The problem isn't the length as I said, the problem is that there is no convention for the naming
of these. No pattern that you can follow, you usually have to go to the start of the function
(which can be hundreds of lines up, depending on what you are looking through) and see how it gets
assigned (And interacts with other variables that have the exact same problem)
I'd try to replace a lot of these names in a patch (I have already done so for sh_main in a patch
that I didn't send because getting the compiler to shut up was more important, will send the patch
when I'm sure it won't conflict with anything) but to be accurate in renaming I have to understand
what they are, (The problem I'm trying to fix) which requires either better names to begin with or
extensive auditing and debug printf-ing of the code that takes up a lot of time.
- Oliver Webb aquahobbyist at proton.me
More information about the Toybox
mailing list