6 ms·

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.

epx 11y ago
This is the only item that I disagree with the Linux coding style ("Do not unnecessarily use braces where a single statement will do.")
- TickleSteve 11y ago
  There is a lot to dislike in the Linux coding style... and yes, this is one.
kllrnohj 11y ago
People will make mistakes. Policy does not prevent that. Tools do.
- gravypod 11y ago
  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.
  - marcosdumay 11y ago
    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.
- kbenson 11y ago
  Tools won't prevent it without a policy to enforce. It's still ultimately up to us to determine the rules the tools follow.
lmm 11y ago
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)
- return0 11y ago
  > add visual clutter and reduce readability one person's clutter is another person's markers. I wouldn't agree they reduce readability
- stephenr 11y ago
  > 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.
  - lmm 11y ago
    > 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.
iainmerrick 11y ago
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.
- gravypod 11y ago
  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.
  - iainmerrick 11y ago
    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.
    - mikaraento 11y ago
      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
      - gravypod 11y ago
        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.
    - jjuhl 11y ago
      Ever heard of clang-format? http://clang.llvm.org/docs/ClangFormat.html http://clang.llvm.org/docs/ClangFormat.html
AdmiralAsshat 11y ago
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.
- asendra 11y ago
  How about this one (true) ? foo() : bar();
  - _0ffh 11y ago
    They will send the Inquisition after you for that one! :)
    - asendra 11y ago
      Well I'm Spanish after all ;)
- gravypod 11y ago
  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..
- iainmerrick 11y ago
  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.)
- pif 11y ago
  Btw, I suppose you wanted to add baz() after bar(). Adding it after foo(), as you did, will not compile.
MichaelBurge 11y ago
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?
- gravypod 11y ago
  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.
  - mikeash 11y ago
    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.
    - gravypod 11y ago
      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.
pkaye 11y ago
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.
- gravypod 11y ago
  We have all learned that lesson one way or another. It's never fun.
ConceptJunkie 11y ago
I've done this for as many years as I can remember. Unfortunately, almost no one else I work with does.

epx 11y ago

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

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

kllrnohj 11y ago

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

gravypod 11y ago
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.
- marcosdumay 11y ago
  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.
kbenson 11y ago
Tools won't prevent it without a policy to enforce. It's still ultimately up to us to determine the rules the tools follow.

lmm 11y ago

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)

return0 11y ago
> add visual clutter and reduce readability one person's clutter is another person's markers. I wouldn't agree they reduce readability
stephenr 11y ago
> 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.
- lmm 11y ago
  > 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.

iainmerrick 11y ago

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.

gravypod 11y ago
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.
- iainmerrick 11y ago
  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.
  - mikaraento 11y ago
    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
    - gravypod 11y ago
      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.
  - jjuhl 11y ago
    Ever heard of clang-format? http://clang.llvm.org/docs/ClangFormat.html http://clang.llvm.org/docs/ClangFormat.html

AdmiralAsshat 11y ago

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.

asendra 11y ago
How about this one (true) ? foo() : bar();
- _0ffh 11y ago
  They will send the Inquisition after you for that one! :)
  - asendra 11y ago
    Well I'm Spanish after all ;)
gravypod 11y ago
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..
iainmerrick 11y ago
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.)
pif 11y ago
Btw, I suppose you wanted to add baz() after bar(). Adding it after foo(), as you did, will not compile.

MichaelBurge 11y ago

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?

gravypod 11y ago
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.
- mikeash 11y ago
  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.
  - gravypod 11y ago
    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.

pkaye 11y ago

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.

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

ConceptJunkie 11y ago

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