This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by erichkeane on Jun 1 2020, 12:17 PM.

Details

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.

Diff Detail

Event Timeline

erichkeane created this revision.Jun 1 2020, 12:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 1 2020, 12:17 PM

SGTM

llvm/docs/CodingStandards.rst
1573

case too

jroelofs accepted this revision.Jun 1 2020, 12:46 PM

LGTM

llvm/docs/CodingStandards.rst
1582

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

This revision is now accepted and ready to land.Jun 1 2020, 12:46 PM
hubert.reinterpretcast added inline comments.
llvm/docs/CodingStandards.rst
1580

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.

1592

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;
hubert.reinterpretcast requested changes to this revision.Jun 1 2020, 12:58 PM
This revision now requires changes to proceed.Jun 1 2020, 12:58 PM

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

llvm/docs/CodingStandards.rst
1580

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

I would agree to that.

1592

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

erichkeane marked 4 inline comments as done.Jun 1 2020, 1:31 PM
erichkeane added inline comments.
llvm/docs/CodingStandards.rst
1573

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.

1580

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

1592

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 updated this revision to Diff 267714.Jun 1 2020, 1:32 PM

Attempt improvements based on feedback.

llvm/docs/CodingStandards.rst
1576

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

1577

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

1578

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.

1581

"loop" should not be in code font.

1581

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

1582

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

1598

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

erichkeane marked 6 inline comments as done.Jun 2 2020, 5:34 AM
erichkeane added inline comments.
llvm/docs/CodingStandards.rst
1578

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

erichkeane updated this revision to Diff 267856.Jun 2 2020, 5:37 AM
llvm/docs/CodingStandards.rst
1578

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

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 added a subscriber: arsenm.Jun 3 2020, 6:05 AM
arsenm added inline comments.
llvm/docs/CodingStandards.rst
1573

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

1603

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 marked 5 inline comments as done.Jun 3 2020, 6:41 AM
erichkeane added inline comments.
llvm/docs/CodingStandards.rst
1573

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.

1603

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 updated this revision to Diff 268175.Jun 3 2020, 6:43 AM

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.

llvm/docs/CodingStandards.rst
1576

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

erichkeane updated this revision to Diff 268433.Jun 4 2020, 5:18 AM
erichkeane marked an inline comment as done.

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

llvm/docs/CodingStandards.rst
1603

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.

This revision is now accepted and ready to land.Jun 4 2020, 7:06 AM
This revision was automatically updated to reflect the committed changes.
smeenai added inline comments.
llvm/docs/CodingStandards.rst
1601

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 marked an inline comment as done.Jun 15 2020, 12:41 PM
erichkeane added inline comments.
llvm/docs/CodingStandards.rst
1601

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.