Hacker News new | ask | show | jobs
by doctorpangloss 2682 days ago
Is there a reason the kernel style doesn't always require curly braces after "if" statements?
5 comments

Note 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.
"Do not unnecessarily use braces where a single statement will do."

https://www.kernel.org/doc/html/v4.10/process/coding-style.h...

A rule that should be removed from every coding style doc for every c-like language on the planet.
Not 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-wmislead...

Why? Because Apple had a critical vuln one time which was made slightly harder to spot because of it?
When 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.

The 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; ..."?

> 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. :-)

You 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)
If 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.
Nonsense.
It feels good to make decisions for every coding style on the planet, doesn't it?
I follow the opposite rule. If any branch of a compounded `if/else` is braced, then so shall all of the others.
A 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:"
in personal code, sure

in organizational code that will be looked at by dozens of people potentially over decades, no

> 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?

> 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.

> 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.

I don't care how big your screen is. There is a limit to how many lines can fit in my field of vision.
One day someone should try shrinking brace-only lines to half height in graphical editors.
There 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=OmarRwem...
The 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.

You say it's absurd, but many people will instinctively remember goto fail: https://www.imperialviolet.org/2014/02/22/applebug.html
clang does not have such a warning, though clang-tidy and GCC do:

https://bugs.llvm.org/show_bug.cgi?id=18938

Agreed 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.
It'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.
braces won't save you from

if (something); { do_something(); }

A basic linter would save you from both problems, assuming they first remove the offending code-style rule.
A 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.

> 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.

This. A hundred times this. Yes, the linter will only save you from basic errors...but indeed, most of the errors are of this type. With a linter, they can be caught even before making it into source control, much less into code review.
A basic linter will hopefully flag two unrelated statements on the same source line...
A decent linter would also warn about the empty block.
if the curly brackets of an if statement are unnecessary then that should also be an error.
If I saw that, I'd ask why someone put the code block on the same line as the conditional.
It also won't save you from

    if(something);
    {
       do_something();
    }
A linter wouldn't point out that there's a conditional with no code block? Or a code block being defined unnecessarily?
Sure, but it'll also catch things like

   if (a)
       b;
       c;
So, the brace style really doesn't matter when it comes to tools catching mistakes. If you turn on the warnings and linters, they'll catch all the common bracing errors. If you don't, most of the suggestions here don't really help much.
> Or a code block being defined unnecessarily?

Of course it depends on your configuration, but my organization occasionally uses code blocks to denote things like critical sections in baremetal applications. So you'd have

  DINT;
  {
      // code that should not be interrupted
  }
  EINT;
I would expect a linter to call out a conditional with no code block, but our workflow is such that we've usually tested things (at least in a manual testing capacity) prior to running any static analysis, so I don't know for sure.
Yeah. We have spent some time configuring our linters so they match our organization's style - and syntax exists to exclude specific exceptional-but-correct usages from being flagged - but now, if the linter raises any flags, they're almost 100% signal, with only an occasional false alarm*. The rules don't need to be universally applicable :)

(The caveat being, of course, that the linter is only a helper tool - "linter doesn't complain" doesn't guarantee correctness, just that "linter complains" is a fairly certain sign of incorrectness)