[PATCH] D82594: Clarify a bit the guideline on omitting braces, including more examples (NFC)

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: llvm/docs/CodingStandards.rst:1580
+
+We consider that readability is harmed when omitting the brace in presence of a
+single statement when accompanied by a comment (assuming the comment can't be
----------------
s/presence/the presence/;


================
Comment at: llvm/docs/CodingStandards.rst:1581
+We consider that readability is harmed when omitting the brace in presence of a
+single statement when accompanied by a comment (assuming the comment can't be
+hoisted above the ``if`` or loop statement, see below).
----------------
s/when/that is/;


================
Comment at: llvm/docs/CodingStandards.rst:1588
 This list is not exhaustive, for example, readability is also harmed if an
-``if``/``else`` chain starts using braced bodies partway through and does not continue
-on with braced bodies.
+``if``/``else`` chain does not use braced bodies everywhere or nowhere
+consistently, with complex conditionals, deep nesting, etc. Some examples below
----------------
To reduce ambiguity as to whether "everywhere" refers to using braces whenever possible within the bodies or just around the bodies themselves, I suggest:
[ ... ] chain does not consistently use braced bodies for either all or none of its members, [ ... ]



================
Comment at: llvm/docs/CodingStandards.rst:1589
+``if``/``else`` chain does not use braced bodies everywhere or nowhere
+consistently, with complex conditionals, deep nesting, etc. Some examples below
+intend to provide some guidelines.
----------------
s/Some examples/The examples/;


================
Comment at: llvm/docs/CodingStandards.rst:1606
+
+  // Use braces if the comment in the body can't be move here above the `if`.
+  if (isa<VarDecl>(D)) {
----------------
s/move/moved/;


================
Comment at: llvm/docs/CodingStandards.rst:1608
+  if (isa<VarDecl>(D)) {
+    // In this else case, it is necessary that we explain the situation with this
+    // surprisingly long comment, so it would be unclear without the braces whether
----------------
The comment still refers to itself as being for an "else case". It would help if the comment better matches an example of a comment that cannot be moved above the `if`. A plausible scenario would be one where there is a long comment above the `if` that explains the need to check for a condition in order to do something special and a long comment within the `if` that explains the specific implementation.


================
Comment at: llvm/docs/CodingStandards.rst:1648
+
+  // Use braces on the outer block because more than two levels of nesting.
+  if (isa<FunctionDecl>(D)) {
----------------
s/more/there are more/;


================
Comment at: llvm/docs/CodingStandards.rst:1658
+    if(shouldProcess(D))
+      handleVarDecl(D);
+  }
----------------
Should there be an `else` here? Otherwise, I think this example is very similar to the one that demonstrates a "potential dangling else situation".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82594