Hacker News Reader

Comment by gravypod | Hacker News Reader

This seems like it could have been avoided by people using braces around every block.

Omitting braces in this case leads to a lot of problems.

This warning is a practical solution to a real problem (of which the famous "goto fail" is an example).

Using braces everywhere is definitely good style, but it isn't a practical solution because there's a ton of existing C/C++ code that doesn't use braces.

Wouldn't it make sense to update that as you change the code? If you edit an if statement, you should add the braces in.

It would make sure that no new errors are introduced due to this.

That's a good policy for code reviews, but again it's hard to do it correctly in a tool.

The compiler would need to require braces in new code, but allow brace-free style in old code; you'd need to integrate it with the source control system so it knows what code is new. Doable, but tricky.

I've found improving style checks file-by-file to be a reasonable approach:

- all new files are checked

- old files are put in a whitelist for the style checker, the submitter is supposed to remove the file from the whitelist when signifcant changes are made

I'm very glad to see that I'm not the only one who does this. I wanted to see if others felt that this process worked.

It is good to hear others are doing it.

Ever heard of clang-format?

http://clang.llvm.org/docs/ClangFormat.html

Of course, that's the (silent) assumption. But humans sometimes fail to do this, which is why the warning is helpful. Very nice to see this in GCC.

Automatic brace insertion program with helper scripts: https://matt.sh/howto-c#_formatting

It is never an error to insert braces, and the rules for inserting braces are fully deterministic in C.

I'm in favor of braces around everything, but to be honest even I will occasionally use the indent if I only expect one action. e.g.

  if (true)
    foo();
  else
    bar();

This looks prettier to my eyes than

  if(true){
    foo();
  } else {
    bar();
  }

However, I might not be the last person to touch the code. My coworker might come later and add:

  if (true)
    foo();   
  else
    bar();
    baz();

And hence the indent problem.

I always opt out of the "no braces needed thing," it is my feeling that you should stick to the more "all around" method of writing something so you never need to switch back and forth..

Even if you ignore safety, braces are still better because if need to edit the code later, you won't need to change as many lines, so the source control history will be cleaner. (Similar to adding a trailing "," to lists that don't strictly need it.)

Btw, I suppose you wanted to add baz() after bar(). Adding it after foo(), as you did, will not compile.

Thanks, I fixed it.

Why should the inability to write basic C preclude me from being allowed to pontificate about brace style like the greats?

Prettier but potentially dangerous? That's the core of the problem, I believe.

I agree. I don't care much about if something looks good, but rather if it works well.

Everything is potentially dangerous. I mean, pointers, anyone? So many things that can go wrong with those.

And besides, code is for humans to read and only incidentally for computers to execute. Might as well optimize for prettiness.

This is the poster case for "code is for humans to read, so it shouldn't be easy to mis-read." Braceless misindented ifs are everything except easy to read.

I agree. But your last example won't compile anyway. Though this would:

  if (true)
    foo();
  else
    bar();
    baz();

But I have to admit if you ever actually do that, you have failed to fundamentally understand how your code is executed. No braces means one statement. Period.

In these cases languages which are auto formatted (C#, go-fmt, etc) likely have an advantage where these problems stick out more obviously.

But I have to admit if you ever actually do that, you have failed to fundamentally understand how your code is executed

I would say I have a pretty good understanding of how C & my code in general works, I'm currently trying to create a threadpool with support for co-routines/yielding to other threads in userspace. Nothing super fancy, but not something you can do without understanding how code is actually executed ;)

But I still lost an hours work last week because I hadn't put a brace after an if statement and when I came back to it, I didn't notice the lack of braces and put an extra statement behind it. This is why I usually stick to putting braces around everything.

In cases where I have only one statement I often drop the braces, but I also drop the newline.

  if (true) foo();
  else bar();

Not much room for a confused baz() here, or so I hope.

This is actually a worse solution. When speed reading code having control statements on their own lines is far more beneficial as you can at a glance, using the indentation, easily see what the flow of execution is.

You might find it worse, I don't. You might not be able to parse this quickly, I am.

No holy wars about style please! I was just offering an alternative that works well for me.

How about this one

(true) ? foo() : bar();

They will send the Inquisition after you for that one! :)

Well I'm Spanish after all ;)

Personally I'm a fan of braces everywhere (though I prefer Allman style), but the one area where I've given up is precondition checks at the start of a function.

Any style with braces looks like a cluttered mess compared to

    if (foo)
      throw ...
    if (bar)
      throw ...
    if (baz)
      throw ...

But I strictly limit that to the beginning of a function, and only `if (x) [throw|return] y;`

> Personally I'm a fan of braces everywhere (though I prefer Allman style)

you monster.

So much wasted vertical space. We use that at work and I am not at all a fan.

That's why smart programmers use 2x monitors in portrait mode.

Try it once. You'll never go back.

I think I don't really have that bar() baz() problem because I use blank lines to separate blocks of code from each other. And also because as a heathen I use Allman style not K&R.

Suppose Debian decides to hire you as their new release maintainer. They want to ensure that bugs such as the OpenSSL one mentioned in the article never happen again. What actions would you take before next month's release to attempt to catch it?

I see from your comment that you might hire a team of developers to go through a critical path of important C/C++ packages checking for style violations. Any code found missing braces will have a patch submitted upstream to correct the errant style. Any package that declines the style changes would be removed from Debian. Any developer you hire that misses style violations will be removed from the team.

Another approach might be: Turn on the warning mentioned in the article and manually review any cases that come up. That still might generate a lot of cases, but it at least seems possible.

Is there another actionable interpretation of your comment that I'm missing?

No. That is not what I mean. If you look at my response to iainmerric you will see that I don't feel you should change code that is working.

This type of bug is largely introduced from someone who is going in and changing a section of code. My philosophy is that if you are editing a section of code, you should update the braces in that section along with your current patch. Over time you see a larger and larger drop in these kinds of bugs as people always, on my team due to the habit of fixing braces and a matching drop from outside contributes due to a larger and larger portion of the code matching the style guide.

That seems like a somewhat different case, though. Your approach will help prevent new bugs like this from being introduced, but won't help you against existing examples of it in areas you're not currently working on. And just because it's been around a long time doesn't mean it's "working." Goto fail showed that the bugs can be subtle and easily missed for a long time.

Yes, but from what I understand this bug is more commonly introduced in code that is being updated.

Notice that I said "avoided", not "fixed". That is because I am rather focused on making code that is maintainable.

I posted this original comment to have a dialogue about something I include in my programming practices. I wanted to see how other people viewed this as a development technique.

You didn't say it explicitly, but your original comment came off more like "This option is unnecessary and should be removed from gcc, since we can simply enforce a style of using curly braces everywhere"

C doesn't do this, but I like this feature from Perl for single-statement checks:

    return if is_red($traffic_light);

    return unless is_green($traffic_light);

In C, I usually put the early returns on their own line without curly braces, but place them all together at the start of the function to make it easier to spot inconsistencies.

I love that feature of Perl, and to be explicitly clear to those that are unfamiliar with it, Perl does not allow unbraced single line if's in the normal sense (pre-conditional ifs), and does not allow braced post-conditional ifs. For example, these are valid:

    if ( $foo ) {
      bar();
      baa();
    }
    quux() if $foo;

And these are not:

    if ( $foo ) bar();
    { bar(); baz(); } if $foo;

This is the only item that I disagree with the Linux coding style ("Do not unnecessarily use braces where a single statement will do.")

There is a lot to dislike in the Linux coding style... and yes, this is one.

I remember doing this mistake back in college. Was required to program and demo the B-Tree backend for a mini database. Things were very well tested for tons of random sets of data but while waiting for the my turn to demo, added one line of debug print on an "if" statement which caused the now dangling statement to be always executed. This caused a subtle bug that the professor caught in his testing. Sucked because I had everything correct up to that point. Learned my lesson about last might changes and coding style at that point.

We have all learned that lesson one way or another. It's never fun.

People will make mistakes. Policy does not prevent that. Tools do.

Not in every case. I'd say that policy does help in some cases and tools do in others.

What I often see is people just ignore the output from tooling and commit anyway.

No method of prevention is perfect, but everything you do can help to increase stability in projects.

A policy of "never ignore this warning" has a much better chance of being followed and removing the bugs than a policy of "always put brace on code blocks".

Yes, both are policy, but they are different kinds.

Of course, banishing single line blocks at the compiler would be infallible, but it's also not viable.

Tools won't prevent it without a policy to enforce. It's still ultimately up to us to determine the rules the tools follow.

I've done this for as many years as I can remember. Unfortunately, almost no one else I work with does.

Extra braces add visual clutter and reduce readability, especially when they mean a function no longer fits on one screen. So if there's a way to eliminate that kind of bug without having to add more braces then I'm all in favour of it.

(Personally my preference would be a linter that automatically runs on checkin, and refuses commits that do not conform to the style guide)

> add visual clutter and reduce readability

one person's clutter is another person's markers. I wouldn't agree they reduce readability

> when they mean a function no longer fits on one screen

What screen size? What resolution? What ide/editor window size? What font face & size? With or without soft line wrapping?

Honestly arguments about how "pretty" code is are fucking ridiculous. Despite what someone said, the purpose of source code is not to be "read" with "running" as a secondary task. That literally only applies to code written purely for educational purposes.

The purpose of code is to achieve the goals of the software as efficiently as possible. Efficiency is not just about speed - a fast but unreliable program is not efficient.

> What screen size? What resolution? What ide/editor window size? What font face & size? With or without soft line wrapping?

Does it matter? There will be a point at which an extra line makes the difference. And particularly with modern screen shapes, vertical space is at much more of a premium than horizontal space.

> Honestly arguments about how "pretty" code is are fucking ridiculous. Despite what someone said, the purpose of source code is not to be "read" with "running" as a secondary task. That literally only applies to code written purely for educational purposes. > The purpose of code is to achieve the goals of the software as efficiently as possible. Efficiency is not just about speed - a fast but unreliable program is not efficient.

The ability to understand software is vital to real-world usefulness though. Requirements change all the time, and so the ability to make changes to software is vital - and to effect desired changes to code you must first understand it.

> And particularly with modern screen shapes, vertical space is at much more of a premium than horizontal space.

What? Modern 10:16 screens give more lines than 3:4. 900x1440 gets like 70 lines with 100 columns at a decent font size, better than around 60 lines with 110 columns with 960x1280.

Good luck using your laptop vertically.

As I said in the other thread I'm not talking about readability.

The exact term used was pretty.

> There will be a point at which an extra line makes the difference

So why not remove all blank lines too. They're less important for reducing issues that single line/braceless flow control blocks can cause.

> So why not remove all blank lines too. They're less important for reducing issues that single line/braceless flow control blocks can cause.

By and large I do. But I don't think what you say is actually true. Given the choice between:

    stepa1
    if(something)
      stepa2
    stepa3

    stepb1
    stepb2
    stepb3

and

    stepa1
    if(something) {
      stepa2
    }
    stepa3
    stepb1
    stepb2
    stepb3

(same number of lines), I think the former is often more readable - stepa2 is visually separated in either case.

If the code can't be read, then it really doesn't matter how it runs because no one will understand it well. Trust me. Some of the code I work with has this problem. I fix it when I can, but there's just too much to fix it all.

The only code where readability isn't as important as functionality is finished code, and we all know that code is never finished.

I didn't say readability doesn't matter.

I'm talking about people who skip braces on single statements, skip semicolons in JavaScript, etc because it "looks prettier" without them

Reply on news.ycombinator.com