6 ms·
Looks like this is the fix: https://github.com/torvalds/linux/commit/9060cb719e61b685ec0102574e10337fa5f445ea https://github.com/torvalds/linux/commit/9060cb71
by ptrincr 8y ago
Looks like this is the fix:
https://github.com/torvalds/linux/commit/9060cb719e61b685ec0102574e10337fa5f445ea https://github.com/torvalds/linux/commit/9060cb719e61b685ec0...
- doctorpangloss 8y agoIs there a reason the kernel style doesn't always require curly braces after "if" statements?
- a-wu 8y ago"Do not unnecessarily use braces where a single statement will do." https://www.kernel.org/doc/html/v4.10/process/coding-style.html#placing-braces-and-spaces https://www.kernel.org/doc/html/v4.10/process/coding-style.h...
- kgwxd 8y agoA rule that should be removed from every coding style doc for every c-like language on the planet.
- shawnz 8y agoWhy? Because Apple had a critical vuln one time which was made slightly harder to spot because of it?
- thaumaturgy 8y agoWhen drafting a style guide, one of your goals is to make the code as uniform as possible. It removes ambiguity and makes it easier to enforce the rules of the style guide, hopefully with automated tooling. Mandatory bracing is a step towards more uniformity, and makes additional statements in a conditional block always safe, and at the cost of just one extra line in the code. It also makes your commits just a little bit smaller; if you do add more lines to a conditional block that was previously braceless, now you just get the new lines in the diff, instead of new lines + opening brace + closing brace. Cowboy coding of course scoffs at all this and people have different values when making tradeoffs between readability and concision, but there are good reasons for enforcing mandatory braces.
- CapacitorSet 8y agoThe uniformity argument becomes rather silly when applied to other constructs. Would you ditch switch-cases in favor of if-else chains (leaving aside fallthrough/Duff's/etc for a moment) because the latter is uniform with existing constructs? Would you ditch "for (int i = 0; ..." in favor of "int i; for (i = 0; ..."?
- thaumaturgy 8y ago> Would you ditch switch-cases in favor of if-else chains No. You're talking about applying style rules for if-else conditionals to statements which aren't if-else, which isn't what I was talking about. This is why the Linux kernel style guide, OpenBSD style, PSR, etc. all have separate guidelines for switch-case along with guidelines for mandatory bracing in if-else. > Would you ditch "for (int i = 0; ..." in favor of "int i; for (i = 0; ..."? There isn't a clear-cut rule about this convention in style guides. That's probably in part because the behavior in your two examples is different: in the first case (in C++), i only exists within the scope of the for loop, whereas in the latter case it will continue to exist outside the scope of the for loop. Whichever one is appropriate would probably depend on context that's outside the scope of a style guide. They're called style guides, after all, not Programming Rules of Law. :-)
- yjftsjthsd-h 8y agoYou can think of 1 high profile incident. But the failure is easy to overlook. Why do you think that there aren't more? (Also, I'm pretty sure I've seen others like this in the news; it's not "one time" caught)
- Piskvorrr 8y agoIf I had a penny for each of these coding errors I have personally fixed, across various projects and languages, I would probably have...about a dollar. Which, IMNSHO, allows me to sufficiently extrapolate that this error is extremely widespread, and to speculate that it might be lurking in other critical locations.
- jcoffland 8y agoNonsense.
- Gibbon1 8y agoMissing curly braces is something that only confuses extremely green or mediocre programmers.
- sangnoir 8y agoYes, all the more reason to disallow it in the standard so it is caught by the linter: green/mediocre programmers write a lot of code (which you most likely use). You can also add "tired programmers" to your list of easily-confused programmers, and that pretty much covers everyone at some point.
- scott_s 8y agoThen color me green and mediocre because I have been bit by this before. Or maybe we should have the humility to realize none of us are above silly mistakes.
- kurtisc 8y agoThe majority of programmers are, by definition, mediocre or worse. MISRA C specifically bans this code style.
- Gibbon1 8y agoMISRA C also bans function pointers, goto, multiple returns and continue statements. Doesn't mandate static analysis tools either. Nuff said about that.
- gmueckl 8y agoStatic analysis tooling is outside the scope of these MISRA conventions. And yes, the language features you list are recommended against for extremely good reasons. If you know better, you can try to convince the examiners during your certification process that your coding style is superior. Good luck!
- Gibbon1 8y ago> certification process Spits tea on my keyboard.
- bodyfour 8y agoNot really needed. Newer versions of gcc support "-Wmisleading-indentation" which is good at catching misuse https://developers.redhat.com/blog/2016/02/26/gcc-6-wmisleading-indentation-vs-goto-fail/ https://developers.redhat.com/blog/2016/02/26/gcc-6-wmislead...
- alexeiz 8y agoIt feels good to make decisions for every coding style on the planet, doesn't it?
- kazinator 8y agoI follow the opposite rule. If any branch of a compounded `if/else` is braced, then so shall all of the others.
- jlg23 8y agoA little further down in the document linked: "This does not apply if only one branch of a conditional statement is a single statement; in the latter case use braces in both branches:"
- kevindong 8y ago> but all right-thinking people know that (a) K&R are right and (b) K&R are right > ... > Rationale: K&R. I REALLY do not like just calling something correct because it was in the K&R book. > Also, note that this brace-placement also minimizes the number of empty (or almost empty) lines, without any loss of readability. Thus, as the supply of new-lines on your screen is not a renewable resource (think 25-line terminal screens here), you have more empty lines to put comments on. Is that really an acceptable justification in the era of cheap 4K displays?
- jstimpfle 8y ago> I REALLY do not like just calling something correct because it was in the K&R book. The statement you quoted is simply saying that the author agrees with K&R. Nothing wrong with it.
- gizmo686 8y agoI don't care how big your screen is. There is a limit to how many lines can fit in my field of vision.
- pjc50 8y agoOne day someone should try shrinking brace-only lines to half height in graphical editors.
- Karliss 8y agoThere are already editor plugins doing that, compressing empty lines is slightly more common but some also support braces. For example https://marketplace.visualstudio.com/items?itemName=OmarRwemi.LinePress https://marketplace.visualstudio.com/items?itemName=OmarRwem...
- kanox 8y ago> Is that really an acceptable justification in the era of cheap 4K displays? Keeping line count down help readability. Some people use high resolution with small fonts but that makes my eyes hurt so my terminal has 45 lines when maximized.
- opportune 8y agoin personal code, sure in organizational code that will be looked at by dozens of people potentially over decades, no
- potiuper 8y agobraces won't save you from if (something); { do_something(); }
- kgwxd 8y agoA basic linter would save you from both problems, assuming they first remove the offending code-style rule.
- potiuper 8y agoA basic linter does not resolve the semantic question. The linter could be satisfied by changing the code syntax given the rule to always include curly brackets after an if to: if (something) {}; { do_something(); } But, the question is the empty bracket correct or should the contents of the subsequent scope be cut into the scope of the if bracket or copied into it or should the if condition be removed altogether? Code styles do not change the intent and always requiring {} after an if is unnecessary for single statements given the intent is correct: if (side_effect()); else { do_something(); } Alternative syntax styles for the same semantics do not clarify the intent of a program.
- aidenn0 8y agoA basic linter will hopefully flag two unrelated statements on the same source line...
- Sohcahtoa82 8y ago> A basic linter does not resolve the semantic question. That's not the job of the linter, that's the job of the engineer. > The linter could be satisfied by changing the code syntax [...] What you're suggesting is the equivalent of turning your car stereo up so that you don't hear the strange noise your car is making. > But, the question is the empty bracket correct or should the contents of the subsequent scope be cut into the scope of the if bracket? That's the job of the engineer to figure out. The linter is only supposed to raise the flag, not come up with the answer.
- kgwxd 8y agoAgreed it should, but that doesn't seem to have been the cause of the issue. Looks there there was originally only one statement after the if but a new one had been added, so the braces were also added.
- h1d 8y agoIt's a good practice to have the braces all the time to avoid making mistakes as it's easier to add and remove lines in it.
- gvb 8y agoNote that the problem was not missing curly braces, it was the missing line sock->sk = NULL; The curly braces were added as well because the single statement "if" turned into a two statement block.
- deleted 8y ago[deleted]
- deleted 8y ago[deleted]
- vortico 8y agoThe only problem that would solve is braindead code like if (something) doThis(); andThat(); which is absolutely absurd to pass even basic code review. Even most compilers (clang, GCC, etc.) will warn you about that. What omitting curly braces after `if` statements does do is make code ~1% more readable, which can have massive cumulative positive results in security and stability with a multi-million line codebase.
- Jach 8y agoYou say it's absurd, but many people will instinctively remember goto fail: https://www.imperialviolet.org/2014/02/22/applebug.html https://www.imperialviolet.org/2014/02/22/applebug.html
- 1over137 8y agoclang does not have such a warning, though clang-tidy and GCC do: https://bugs.llvm.org/show_bug.cgi?id=18938 https://bugs.llvm.org/show_bug.cgi?id=18938
- topspin 8y agoIn there over 8 years.