Hacker News new | ask | show | jobs
by DSMan195276 4220 days ago
IMO, the bug was a much deeper issue then simply not putting braces on if statements. It doesn't matter if the code becomes this:

    if (condition) {
        goto fail;
    }
        goto fail;
if nobody looks at the commit. Don't get me wrong though, braces on if's do help for making cleaner patches, so there is a valid reason to request braces. You should never rely on them to fix these types of bugs though, that's bound to come back and bite you. In general, having a proper system for submitting and approving patches (Like the Kernel has) will allow you to avoid errors like this one.
1 comments

I certainly don't disagree with you on the importance of process. I was going to mention how this probably would never be an issue for the kernel.

To me, though, requiring braces would make it much easier to spot any such problem at any point in the development process (writing, debugging, reviewing, maintenance) such that the extra line per conditional would be well worth it in all cases, not to mention making edits easier.

I think it's worth noting that a pretty big percentage of the if's in the kernel are one line. I'm not particularly tied to one opinion or the other, I'll do whatever fits with the project (Though I do tend to use the one line if syntax for personal projects). But, I personally like them in the kernel's source simply because one line if's are so common and mostly encouraged. IMO, a better solution is to use a context-aware patch system, rather then line-based patches. That brings it's own set of problems though unfortunately.

That said, I think the argument does apply in that some pieces of the kernel don't strictly follow the kernel style, and the fact that braces aren't enforced leads to some uglier pieces of code [0] being allowed despite not strictly adhering to the style.

[0] https://github.com/torvalds/linux/blob/master/kernel/groups....