[PATCH] D80947: Add to the Coding Standard our that single-line bodies omit braces

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: llvm/docs/CodingStandards.rst:1582
+clear that it is a single line. Note that comments should only be hoisted for loops and
+'if', and not in 'else if' or 'else', where it would be unclear whether the comment
+belonged to the preceeding condition, or the 'else'.
----------------
jroelofs wrote:
> Elsewhere in this file uses double backticks around keywords that are inline in text.
I believe by "double backticks", @jroelofs meant pairs of double backticks.


================
Comment at: llvm/docs/CodingStandards.rst:1576
+
+When writing the body of an `if`, `else`, or loop statement omit the braces to avoid
+unnecessary and otherwise meaningless code. Braces should be used however
----------------
I believe there should be a comma before "omit".


================
Comment at: llvm/docs/CodingStandards.rst:1577
+When writing the body of an `if`, `else`, or loop statement omit the braces to avoid
+unnecessary and otherwise meaningless code. Braces should be used however
+in cases where it significantly improves readability, such as when the single
----------------
The placement of "however" is awkward here. Starting the sentence with "however" may be preferable to having readers interpret "however" for its meaning of "in whatever fashion".


================
Comment at: llvm/docs/CodingStandards.rst:1578
+unnecessary and otherwise meaningless code. Braces should be used however
+in cases where it significantly improves readability, such as when the single
+statement is accompanied by a comment that loses its meaning if hoisted above the `if`
----------------
I think "navigability" is also negatively affected by omission of braces. This seems to be an aspect of readability this is not always considered. It tends to be easier to consume code in an editor when placing a cursor on a brace highlights the matching brace. If a reviewer in a web interface needed to scroll or "draw a line" to where a loop or if/else chain starts when reaching the end of a block, then the lack of braces is harmful. This would especially be the case if the code was such that having comments after the brace would be helpful.

The use of braces to proactively avoid running into the dangling-else problem should also be permitted or even encouraged.

Replacing the list of cases where braces help readability with a list of cases where omitting braces are harmful may help. We can then enforce braces for some classes of harmful brace-omission and permit braces for other classes.

Examples of "mild" harmful cases can then include mixing of braced and non-braced blocks in an if/else chain.


================
Comment at: llvm/docs/CodingStandards.rst:1581
+or loop statement, or where the single statement is complex enough that it stops being
+clear that it is the sole statement. If said statement is an additional `if` or `loop`,
+it should be considered a single statement for this rule, and follow the rule recursively.
----------------
"loop" should not be in code font.


================
Comment at: llvm/docs/CodingStandards.rst:1581
+or loop statement, or where the single statement is complex enough that it stops being
+clear that it is the sole statement. If said statement is an additional `if` or `loop`,
+it should be considered a single statement for this rule, and follow the rule recursively.
----------------
hubert.reinterpretcast wrote:
> "loop" should not be in code font.
It's more that it stops being easy to identify where the block containing the following statement began.


================
Comment at: llvm/docs/CodingStandards.rst:1598
+    // surprisingly long comment, so it would be unclear without the braces whether
+    // the following statement is in the scope of the `else`.
+    handleOtherDecl(D);
----------------
Note this line is already within a code block, so it does not need the pair-of-double-backticks treatment.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80947/new/

https://reviews.llvm.org/D80947