Slashdot Mirror
Linus Rants About C Programming Semantics (iu.edu)
jones_supa writes: "Christ people. This is just sh*t," begins Linus Torvalds in his message on the Linux Kernel Mailing List. Torvalds is grumpy because some new code added to the IPv6 subsystem has created conflicts. "The conflict I get is due to stupid new gcc header file crap," he writes. "But what makes me upset is that the crap is for completely bogus reasons." The new improved code uses fancy stuff that wants magical built-in compiler support and has silly wrapper functions for when it doesn't exist. Linus provides an alternative that contains a single and understandable conditional, which looks cleaner and generates better code.
It's literally just a few rants out of thousands of friendly messages per year - and Linus only rants if crappy code comes from someone who is trusted by Linus (such as the networking maintainer here) and who should really know better.
Not that this will surprise much of anyone, but the headline is wrong. Linus did not rant about C programming semantics. He ranted about a specific C function and the style used to perform a sanity check.
Goto is a perfectly valid instruction providing you know when to use it. The typical use would be to jump into some teardown code at the bottom of a function or to escape out of some nested loop. Either way provdes more succinct and involves less code than the alternative. I assume every single kernel developer is capable of knowing when best to use it and they wouldn't have to worry about issues with c++ constructors either.
Not to state the obvious, but here's the definition of `overflow_ubus`
static inline bool overflow_usub(unsigned int a, unsigned int b,
unsigned int *res)
{
*res = a - b;
return *res > a ? true : false;
}
So the 2 conditionals from the patch are completely idiotic and wrong.
`overflow_ubus` not only makes an unnecessary assignment to `mtu`, but does a check after that assignment, and then you need another check for it outside of `overflow_ubus`.
In general, the proposed patch conditionals could be rewritten as
unsigned int oldmtu = mtu;
mtu -= hlen + sizeof(struct frag_hdr);
if (mtu > oldmtu || mtu <= 7)
goto fail_toobig;
Now that's ugly!
I have to agree. I was ready to read a melodramatic rant over slightly new semantics, but I instead found a completely justified and reasonable criticism of horribly unreadable (and kind of broken) code. With some extra swearing thrown in for Linusness.
Remember, coders, if you're doing anything with code that will be used by others or reused by you, readability is crucial. I'm not talking about comments unless the code itself needs to be less readable (e.g. Performance in a hot spot). I mean the code itself.
And, by code that others may use or you may re-use, I mean all code.