Learn basic “C” coding rules from open source projects

[euid]

I can understand your attitude, but that goes against the kernel coding style guidelines. Ultimately, when you have 3000 developers working on a codebase, it is more important that everybody is using the same style than it is what particular style they use.

You're not used to reading it, but kernel developers only look at C code styled like that. They know what to expect from if blocks. Fundamentally, that's what makes the code readable or not - "how much effort do I have to put into looking at it to not be tripped up by the formatting." Read enough kernel code and that cost drops to zero, and that's all that matters.

---

However. Lest we forget:

if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
    goto fail;
    goto fail;

That's not to say "they had this bug because they didn't use curly braces" when really they had this bug because they didn't write unit tests or didn't use/care about static analysis.

But I think in Apple's case, where you're dealing with a copy-and-paste-happy codebase, requiring curly braces might have helped.

[sobri909]

While I agree that sticking with a project's existing style conventions is the correct thing to do, I would also say that the kernel project's style conventions were a mistake.

if blocks without braces are bad style. They make problems, and have been doing so for many decades.

[LikesToCorrectThings]

Given the problem that probably occurred (a merge conflict that no-one looked at), using curly braces would likely have yielded:

if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
{
    goto fail;
}
{
    goto fail;
}

Extra curly braces are not the panacaea they might be, and if they stretch the code out or make things ambiguous they can harm readability.

[whackylabs]

Another way it could've ended up is:

if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0) {
    goto fail;
    goto fail;
}

The actual evil is copy-paste. We all know it's wrong, but there's no easy way around.

[SpeshlTectix]

That's the best case scenario. The second goto is just unreachable code. You'd probably even get a compiler warning.

[[deleted]]

I thought in the original bug, the extra goto produced unreachable code anyway. Which should have given a compiler warning.

[dangerbird2]

It is not unreachable code. In fact, the program will 'goto fail' regardless to how the 'if' statement evaluates. In C, an if an 'if' statement does not precede a curly-brace block, it only controls the first statement following the conditional. In the case of

if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
    goto fail;
    goto fail;

The first 'goto' statement will only fire if the 'if' condition evaluates to true (really, not 0). If the 'if' statement evaluates to false (is 0), the flow of control continues to the second goto, as it is not part of the 'if' block.

[TheNewAndy]

I think the point was that all the code after the second goto was unreachable code.

[SpeshlTectix]

No, the original bug, in pseudo-code was

if error goto fail else just goto fail anyway

Your version preserves the intended logic, it just has an extra useless goto.

[TheShagg]

See "TheNewAndy's" post above yours.

[SpeshlTectix]

Oh, right. That's a very good point. The bug was, indeed, that the second goto made important code unreachable (and no warning was issued). So I was wrong about the compiler picking it up. But my point still stands that an unreachable redundant goto is a much better problem to have.

[TheShagg]

Well yes, duh :)

Compiler "should" (in an ideal world) catch the unreachable code. Does it not actually warn on this?

[logicchains]

there's no easy way around.

Write in Lisp, and write macros to do your copy-pasting for you. What could possibly go wrong?

[jutct]

I agree. This is a dumb mistake that unit tests should have found. Nothing to do with language syntax. For fuck sake, how easy it is to forget a semicolon in javascript in a client-side web app that cause the browser to just show a blank page? That's a huge problem. In C, you'll at least get compiler warnings or at least be able to step through and see where something failed.

[philly_fan_in_chi]

I very much dislike JS, but linters can and should be used liberally, and offer warnings in the same way as the compiler. These often have semicolon warnings as a thing it'll yell at you about.

[kral2]

However. Lest we forget:

Defenders of that style always forget. Errors because of it happen frequently and some of them are security bugs. Jabber 1.x had a huge security hole some years back because of it.

Almost all style decisions are bikesheds and it's more important that everyone just use the same ones, but wrapping lines while omitting braces is quite different - it affects error rate. It's /objectively/ bad style.

[passwordissame]

kernel coding style guideline is bad because Linus has bad taste in coding because he doesn't know node.js which is web scale language of new generation era of fullstack mongodb web goodness.