Add to the Coding Standard our that single-line bodies omit braces

[erichkeane]

Summary

This is a rule that seems to have been enforced for the better part of
the decade, so we should document it for new contributors.

[lebedev.ri]

SGTM

llvm/docs/CodingStandards.rst

1572

case too

[jroelofs]

LGTM

llvm/docs/CodingStandards.rst

1581

Elsewhere in this file uses double backticks around keywords that are inline in text.

[hubert.reinterpretcast]

llvm/docs/CodingStandards.rst

1579

I'm happy with the implications of how this is phrased, but I am not sure it was intended. A statement that is not going to be a single line (a loop inside an else) qualifies for braces.

1591

I believe this is an example of bad style. Applying the prose text to the example:
Adding braces in this example to the above bodies do not introduce "meaningless lines of code" as the lines already occur regardless. Adding braces may arguably improve readability.

Say, for the following, the lack of uniformity in the use of braces is a distraction:

if (A)
  zip();
else if (B) {
  foo();
  bar();
} else
  hello;

[jdoerfert]

Thanks for finally writing this down :) Two minor comments.

llvm/docs/CodingStandards.rst

1579

A statement that is not going to be a single line (a loop inside an else) qualifies for braces.

I would agree to that.

1591

I also think a "compound statement" that has braces at some point can/should have them everywhere.

[erichkeane]

llvm/docs/CodingStandards.rst

1572

Case statements are troublesome, right? First, i don't really see that ever being an issue (do we have people unnecessarily doing braces around case statements?), and I consider the conditions on when to use braces in case statements to be different.

1579

Well, I said 'statement', not 'line' to attempt to avoid this :) I'll try a re-word, but I'd love additional suggestions.

1591

I've removed the 'lines of code' from meaningless, but I'd love to hear your suggestion on how to word this rule. I couldn't come up with a set of rules that would be consistent with our current enforcement, and be reasonable.

I considered some rule where 'once an if/else tree gets braces, everything below that point' would have braces, but I didn't have a good wording for it (and I intended to not be novel here compared to enforcement). In your case, the 'else' would have it, and I'd like the 'if' to be optional.

[erichkeane]

Attempt improvements based on feedback.

[hubert.reinterpretcast]

llvm/docs/CodingStandards.rst

1575

I believe there should be a comma before "omit".

1576

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".

1577

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.

1580

"loop" should not be in code font.

1580

It's more that it stops being easy to identify where the block containing the following statement began.

1581

I believe by "double backticks", @jroelofs meant pairs of double backticks.

1597

Note this line is already within a code block, so it does not need the pair-of-double-backticks treatment.

[erichkeane]

llvm/docs/CodingStandards.rst

1577

Can you suggest an alternate wording here? I'm not sure how to capture what you're saying.

[hubert.reinterpretcast]

llvm/docs/CodingStandards.rst

1577

Suggested wording:

When writing the body of an ``if``, ``else``, or loop statement, omit the braces to avoid unnecessary line noise. However, braces should be used in cases where the omission of braces harm the readability and maintainability of the code.

Readability is harmed when a single statement is accompanied by a comment that loses its meaning if hoisted above the ``if`` or loop statement. Similarly, braces should be used when single-statement body is complex enough that it becomes difficult to see where the block containing the following statement began. An ``if``/``else`` chain or a loop is considered a single statement for this rule, and this rule applies recursively. 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.

Maintainability is harmed if the body of an ``if`` ends with a (directly or indirectly) nested ``if`` statement with no ``else``. Braces on the outer ``if`` would help to avoid running into a "dangling else" situation.

1598

I believe my comment was misunderstood. I meant that this part was fine (and there should be something to distinguish between the keywords and the English words). What I meant was that the change implied by another comment of mine (https://reviews.llvm.org/D80947?id=267696#inline-744103) did not apply here.

[arsenm]

llvm/docs/CodingStandards.rst

1572

I would rather just ban single line statements like this and require putting them on the next line.

For the case of cases, especially when combined with the style of not indenting the cases from the switch, omitting braces is really painful. So many times I've produced bad merges and had a hard time figuring out where the double }} at the end is necessary. It would be easier to just always use the braces

1602

This loop should use braces. It covers multiple lines. Omitting braces invariably just increases diffs/merge conflicts when something else is added to the loop body.

This one isn't consistently applied and I've been enforcing the opposite

[erichkeane]

llvm/docs/CodingStandards.rst

1572

It is not my intent in this patch to actually change our coding standard, simply to codify the rule that we've been enforcing for a decade.

1602

This is how we've been enforcing the rule however, can you suggest a change to the wording that you think would match our current enforcement?

[erichkeane]

Apply @hubert.reinterpretcast s spelling.

I'd also like to re-enforce, the purpose of this patch is simply to write down the rule that we've been enforcing for years, I don't think we should get novel with the rules/change how it has been enforced. That seems like it would require an RFC/llvm-dev discussion instead.

[hubert.reinterpretcast]

llvm/docs/CodingStandards.rst

1575

There seems to be some duplication because the "old" version of the write-up is still here.

[hubert.reinterpretcast]

This LGTM. I believe we have not heard back from @arsenm on the response to some of their comments though.

llvm/docs/CodingStandards.rst

1602

Much as it may be seen to be "ideal" that conventions are uniform throughout the project, it is the case that different areas of the code have somewhat of a local convention. The example may lean towards not using braces as the general case, but the wording was done so that areas of code where the observed increase in diffs/merge conflicts occur frequently can use and enforce braces because said occurrences would be indicative of harm caused by the omission of braces.

[smeenai]

llvm/docs/CodingStandards.rst

1600

I'm late to the party here (and I haven't read the entirety of the previous discussion, sorry), but I thought our rule had always been "if one arm of an if/else requires braces, put braces on all of them". In other words, the if and else if here should have braces, since the else does.

[erichkeane]

llvm/docs/CodingStandards.rst

1600

I believe this then has been particularly poorly enforced then. Since the rule was unwritten, its enforcement across the codebase has been extremely inconsistent.

For example, Clang has a number of if/else-if/else chains where braces are omitted JUST in the middle!

I would love for us (the community) to have a discussion to determine what the rule ACTUALLY is, but am not sure this part has been uniformly enforced sufficiently enough to put it in the text without discussion.

llvm/docs/CodingStandards.rst

 expensive.
 
 Use ``auto &`` for values and ``auto *`` for pointers unless you need to make a
 copy.
 
 .. code-block:: c++
 
   // Typically there's no reason to copy.
-  for (const auto &Val : Container) { observe(Val); }
-  for (auto &Val : Container) { Val.change(); }
+  for (const auto &Val : Container) observe(Val);
+  for (auto &Val : Container) Val.change();
 
   // Remove the reference if you really want a new copy.
   for (auto Val : Container) { Val.change(); saveSomewhere(Val); }
 
   // Copy pointers, but make it clear that they're pointers.
-  for (const auto *Ptr : Container) { observe(*Ptr); }
-  for (auto *Ptr : Container) { Ptr->change(); }
+  for (const auto *Ptr : Container) observe(*Ptr);
+  for (auto *Ptr : Container) Ptr->change();
 
 Beware of non-determinism due to ordering of pointers
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 In general, there is no relative ordering among pointers. As a result,
 when unordered containers like sets and maps are used with pointer keys
 the iteration order is undefined. Hence, iterating such containers may
 result in non-deterministic code generation. While the generated code

 predicate isn't true; you have to read to the end of the function to know that
 it returns null.
 
 It is much preferred to format the code like this:
 
 .. code-block:: c++
 
   Value *doSomething(Instruction *I) {
-    // Terminators never need 'something' done to them because ... 
+    // Terminators never need 'something' done to them because ...
     if (I->isTerminator())
       return 0;
 
     // We conservatively avoid transforming instructions with multiple uses
     // because goats like cheese.
     if (!I->hasOneUse())
       return 0;
 
     // This is really just here for example.
     if (!doOtherThing(I))
       return 0;
-    
     ... some long code ....
   }
 
 This fixes these problems.  A similar problem frequently happens in ``for``
 loops.  A silly example is something like this:
 
 .. code-block:: c++
 

 
 .. code-block:: c++
 
   case 'J':
     if (Signed)
       Type = Context.getsigjmp_bufType();
     else
       Type = Context.getjmp_bufType();
-    
     if (Type.isNull()) {
       Error = Signed ? ASTContext::GE_Missing_sigjmp_buf :
                        ASTContext::GE_Missing_jmp_buf;
       return QualType();
     }
     break;
 
 The idea is to reduce indentation and the amount of code you have to keep track
 of when reading the code.
-              
 Turn Predicate Loops into Predicate Functions
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 It is very common to write small loops that just compute a boolean value.  There
 are a number of ways that people commonly write these, but an example of this
 sort of thing is:
 
 .. code-block:: c++

 ``isLValue()``).  Different kinds of declarations have different rules:
 
 * **Type names** (including classes, structs, enums, typedefs, etc) should be
   nouns and start with an upper-case letter (e.g. ``TextFileReader``).
 
 * **Variable names** should be nouns (as they represent state).  The name should
   be camel case, and start with an upper case letter (e.g. ``Leader`` or
   ``Boats``).
-  
 * **Function names** should be verb phrases (as they represent actions), and
   command-like function should be imperative.  The name should be camel case,
   and start with a lower case letter (e.g. ``openFile()`` or ``isFoo()``).
 
 * **Enum declarations** (e.g. ``enum Foo {...}``) are types, so they should
   follow the naming conventions for types.  A common use for enums is as a
   discriminator for a union, or an indicator of a subclass.  When an enum is
   used for something like this, it should have a ``Kind`` suffix
   (e.g. ``ValueKind``).
-  
 * **Enumerators** (e.g. ``enum { Foo, Bar }``) and **public member variables**
   should start with an upper-case letter, just like types.  Unless the
   enumerators are defined in their own small namespace or inside a class,
   enumerators should have a prefix corresponding to the enum declaration name.
   For example, ``enum ValueKind { ... };`` may contain enumerators like
   ``VK_Argument``, ``VK_BasicBlock``, etc.  Enumerators that are just
   convenience constants are exempt from the requirement for a prefix.  For
   instance:
 
   .. code-block:: c++
 
       enum {
         MaxSize = 42,
         Density = 12
       };
-  
 As an exception, classes that mimic STL classes can have member names in STL's
 style of lower-case words separated by underscores (e.g. ``begin()``,
 ``push_back()``, and ``empty()``). Classes that provide multiple
 iterators should add a singular prefix to ``begin()`` and ``end()``
 (e.g. ``global_begin()`` and ``use_begin()``).
 
 Here are some examples:
 

 prefer it.
 
 ``#include <iostream>`` is Forbidden
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 The use of ``#include <iostream>`` in library files is hereby **forbidden**,
 because many common implementations transparently inject a `static constructor`_
 into every translation unit that includes it.
-  
 Note that using the other stream headers (``<sstream>`` for example) is not
 problematic in this regard --- just ``<iostream>``. However, ``raw_ostream``
 provides various APIs that are better performing for almost every use than
 ``std::ostream`` style APIs.
 
 .. note::
 
   New code should always use `raw_ostream`_ for writing, or the
.. code-block:: c++
 
   /// This class represents things that Smith can have an intimate
   /// understanding of and contains the data associated with it.
   class Grokable {
   ...
   public:
     explicit Grokable() { ... }
     virtual ~Grokable() = 0;
-  
     ...
 
   };
 
   } // end namespace knowledge
   } // end namespace llvm
 
 
.. code-block:: c++
   class StringSort {
   ...
   public:
     StringSort(...)
     bool operator<(const char *RHS) const;
   };
   } // end anonymous namespace
 
-  static void runHelper() { 
-    ... 
+  static void runHelper() {
+    ...
   }
 
   bool StringSort::operator<(const char *RHS) const {
     ...
   }
 
 Avoid putting declarations other than classes into anonymous namespaces:
 
.. code-block:: c++
 
   } // end anonymous namespace
 
 When you are looking at "``runHelper``" in the middle of a large C++ file,
 you have no immediate way to tell if this function is local to the file.  In
 contrast, when the function is marked static, you don't need to cross-reference
 faraway places in the file to tell that the function is local.
 
+Don't Use Braces on Simple Single-Statement Bodies of if/else/loop Statements
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+When writing the body of an ``if``, ``else``, or loop statement, omit the braces to
+avoid unnecessary line noise. However, braces should be used in cases where the
+omission of braces harm the readability and maintainability of the code.
+
+Readability is harmed when a single statement is accompanied by a comment that loses
+its meaning if hoisted above the ``if`` or loop statement. Similarly, braces should
+be used when single-statement body is complex enough that it becomes difficult to see
+where the block containing the following statement began. An ``if``/``else`` chain or
+a loop is considered a single statement for this rule, and this rule applies recursively.
+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.
+
+Maintainability is harmed if the body of an ``if`` ends with a (directly or indirectly)
+nested ``if`` statement with no ``else``. Braces on the outer ``if`` would help to avoid
+running into a "dangling else" situation.
+
+
+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``.
+
+.. code-block:: c++
+
+  // Omit the braces, since the body is simple and clearly associated with the if.
+  if (isa<FunctionDecl>(D))
+    handleFunctionDecl(D);
+  else if (isa<VarDecl>(D))
+    handleVarDecl(D);
+  else {
+    // 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 following statement is in the scope of the else.
+    handleOtherDecl(D);
+  }
+
+  // This should also omit braces.  The for loop contains only a single statement,
+  // so it shouldn't have braces.  The if also only contains a single statement (the
+  // for loop), so it also should omit braces.
+  if (isa<FunctionDecl>(D))
+    for(auto *A : D.attrs())
+      handleAttr(A);
+
+
 See Also
 ========
 
 A lot of these comments and recommendations have been culled from other sources.
 Two particularly important books for our work are:
 
 #. `Effective C++
    <https://www.amazon.com/Effective-Specific-Addison-Wesley-Professional-Computing/dp/0321334876>`_