RFC: Improve style for if indentation #9506
Conversation
|
ACK |
|
YES PLEASE. I introduced a bug recently as a result of carelessness which was exacerbated by our coding style on this front. With respect to the one line without braces rule, we should find out if clang format can implement this policy. I see that it can allow one line, but I think it might still require braces there if they're required everywhere else. Also it would be good to work this out before we do any big boost migrations, since there may be changes to fix some of these at the same time. |
|
You might need to update src/.clang-format as well. It contains |
|
ACK |
| - No space after function names; one space after if, for and while. | ||
| - No space after function names; one space after `if`, `for` and `while`. | ||
| - If an `if` only has a single-statement then-clause, it can appear | ||
| on the same line as the if, without braces. In every other case, |
There was a problem hiding this comment.
I am skeptical about the effectiveness of forcing braces in all other cases. Just because it is on a single line does not make all the mistakes disappear.
E.g.
if (!found) ret=-1; return ret;is still wrong (at least wrongly formatted).
And imo an if with a single-statement then-clause on the next line is no worse than on the same line. Also considering that the existing code does not comply with the proposed guideline probably makes it hard to enforce.
... I don't think our issue is insufficient documentation of the problem in the dev notes, but rather the inability of humans to catch all of those errors in code review.
There was a problem hiding this comment.
Personally, I'd be fine with a hard rule that braces must always be used - I don't see much value to the all-on-one-line case.
Having this as a documented rule allows us to complain if it is violated in PRs. New brace-less oneliner blocks are currently being introduced regularly, and (I don't know about others, but) I have held back complaining because we don't have an official policy on it (and also perhaps to some degree because I used to hold the opposite opinion).
|
I am not strictly against the proposed changes, but without a static analyzer (maybe in travis) that requires appropriate formatting of new code, this may not be effective. |
|
Looks good to me. |
74994c6 Improve style w.r.t. if (Pieter Wuille)
74994c6 Improve style w.r.t. if (Pieter Wuille)
74994c6 Improve style w.r.t. if (Pieter Wuille)
74994c6 Improve style w.r.t. if (Pieter Wuille)
8 participants
Lately there have been a few discussions about the indentation/braces recommendations for
ifstatements. I believe the current text in doc/developer-notes.md does not follow best practices (see for example http://www.dwheeler.com/essays/apple-goto-fail.html), and is in fact not being actively encouraged anymore in review either, so I'd like to change it.Feel free to bikeshed this to death.