3 ms·
The rant is about being overly fancy with the code and making it hard to read. If one scans the line, the thing that stands out first is the function being used
by Sanddancer 11y ago
The rant is about being overly fancy with the code and making it hard to read. If one scans the line, the thing that stands out first is the function being used. Not only is it a gccism that has never been used before, but it's a gccism that is used in a poor manner. By copying the "safe" subtraction to the same variable, you've now changed the behavior of the function. Before, if the mtu was too small, it would goto the error handler and not alter the mtu. The new function, if the resulting value is between 0 and 7, will overwrite mtu, match the second conditional, and be shipped off to the error handler with a new, invalid, value.
The second part of cleverness is in fact even more damning than that. The conditional || mtu <= 7 is a siren's call for anyone looking for a cheap optimization. To someone not paying attention, this looks like it could be a nice patch -- take out an or that couldn't possibly be true under any circumstance, and save a few ops on every incoming packet. Bundle it in with other optimizations, and you've got an overflow just waiting to go off.
So no, it's not bikeshedding, it's being too clever for one's own good, and in one line adds a new function call that's just barely been proposed for a standardized version of C, and has a pair of logic errors to boot. In short, ticking off pretty much everything you don't want to do in a line of code.
- heavenlyhash 11y agoThank you: the most comprehensive (and damn near only) comment on the page that talks about the technical decision being represented here.