[PATCH] D126512: [Docs] Clarify the guideline on omitting braces
Owen Pan via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue May 31 09:52:52 PDT 2022
owenpan added inline comments.
================
Comment at: llvm/docs/CodingStandards.rst:1596-1597
-This list is not exhaustive, for example, readability is also harmed if an
+Similarly, braces should be used when a single-statement body can't fit on a
+single line; otherwise, it would be difficult to see where the block containing
+the following statement began. An ``if``/``else`` chain or a loop is considered
----------------
mehdi_amini wrote:
> aaron.ballman wrote:
> > erichkeane wrote:
> > > mehdi_amini wrote:
> > > > owenpan wrote:
> > > > > mehdi_amini wrote:
> > > > > > This is new and seems overly restrictive to me, why this new change?
> > > > > See the second paragraph of the patch summary above. The phrase "complex enough" is too subjective to be useful IMO, but I will withdraw this specific change if you insist.
> > > > Subjective isn't always bad :)
> > > > The coding standard is designed to leave some room indeed when we can't specify a rule that is always clear cut.
> > > I'm not really a fan of this one. I DO really like the 'cascading single-liners', and want to keep those omitting braces. The example @mehdi_amini pointed out is important to me to keep.
> > I'd like to make sure we're all reading this the same way, because I don't see this as being restrictive so much as formalizing what I think many reviewers have been asking for anyway. Here's some examples:
> > ```
> > if (foo) // Old rule: didn't ask for braces, new rule: asks for braces
> > bar(1, 2,
> > 3, 4);
> > ```
> > ```
> > if (foo) // Old rule: didn't ask for braces, new rule: asks for braces
> > bar(1, 2,
> > 3, 4);
> > else // Old rule: didn't ask for braces, new rule: doesn't ask for braces
> > baz(1, 2);
> > ```
> > ```
> > if (foo) // Old rule: didn't ask for braces, new rule: doesn't ask for braces
> > baz(1, 2);
> > else // Old rule: didn't ask for braces, new rule: asks for braces
> > bar(1, 2,
> > 3, 4);
> > ```
> > ```
> > if (foo) // Old rule: didn't ask for braces, new rule: doesn't ask for braces
> > if (bar) // Old rule: didn't ask for braces, new rule: doesn't ask for braces
> > baz(1, 2);
> > ```
> > ```
> > if (foo) // Old rule: didn't ask for braces, new rule: asks for braces
> > if (bar(1, 2,
> > 3, 4)) // Old rule: didn't ask for braces, new rule: doesn't ask for braces
> > baz(1, 2);
> > ```
> > ```
> > if (foo) // Old rule: didn't ask for braces, new rule: asks for braces
> > for (int i = 0; i < 10; ++i) // Old rule: didn't ask for braces, new rule: asks for braces
> > baz(i);
> > ```
> > Am I interpreting this correctly?
> >
> > (FWIW, I personally think it's better for us to err on recommending braces when they're unnecessary in these more complicated situations because that's the safer, more explicit style.)
> Your examples are on point and are the reasons why I see this as a non-trivial policy change. At minima this requires a thread on discourse and a revision separate from the do/while case.
>
> ```
> if (foo) // Old rule: didn't ask for braces, new rule: asks for braces
> bar(1, 2,
> 3, 4);
> else // Old rule: didn't ask for braces, new rule: doesn't ask for braces
> baz(1, 2);
> ```
The new rule asks for braces on the `if` (and because of the existing rule on 1636) and on the `else`.
> ```
> if (foo) // Old rule: didn't ask for braces, new rule: doesn't ask for braces
> baz(1, 2);
> else // Old rule: didn't ask for braces, new rule: asks for braces
> bar(1, 2,
> 3, 4);
> ```
Similarly, the new rule asks for braces on both `if` and `else`.
> ```
> if (foo) // Old rule: didn't ask for braces, new rule: doesn't ask for braces
> if (bar) // Old rule: didn't ask for braces, new rule: doesn't ask for braces
> baz(1, 2);
> ```
Both old and new rules ask for braces on the outer `if`, but the old example didn't really convey that.
> ```
> if (foo) // Old rule: didn't ask for braces, new rule: asks for braces
> if (bar(1, 2,
> 3, 4)) // Old rule: didn't ask for braces, new rule: doesn't ask for braces
> baz(1, 2);
> ```
Both old and new rules ask braces on the outer `if`.
> ```
> if (foo) // Old rule: didn't ask for braces, new rule: asks for braces
> for (int i = 0; i < 10; ++i) // Old rule: didn't ask for braces, new rule: asks for braces
> baz(i);
> ```
Both old and new rules don't ask for **no** braces.
================
Comment at: llvm/docs/CodingStandards.rst:1596-1597
-This list is not exhaustive, for example, readability is also harmed if an
+Similarly, braces should be used when a single-statement body can't fit on a
+single line; otherwise, it would be difficult to see where the block containing
+the following statement began. An ``if``/``else`` chain or a loop is considered
----------------
owenpan wrote:
> mehdi_amini wrote:
> > aaron.ballman wrote:
> > > erichkeane wrote:
> > > > mehdi_amini wrote:
> > > > > owenpan wrote:
> > > > > > mehdi_amini wrote:
> > > > > > > This is new and seems overly restrictive to me, why this new change?
> > > > > > See the second paragraph of the patch summary above. The phrase "complex enough" is too subjective to be useful IMO, but I will withdraw this specific change if you insist.
> > > > > Subjective isn't always bad :)
> > > > > The coding standard is designed to leave some room indeed when we can't specify a rule that is always clear cut.
> > > > I'm not really a fan of this one. I DO really like the 'cascading single-liners', and want to keep those omitting braces. The example @mehdi_amini pointed out is important to me to keep.
> > > I'd like to make sure we're all reading this the same way, because I don't see this as being restrictive so much as formalizing what I think many reviewers have been asking for anyway. Here's some examples:
> > > ```
> > > if (foo) // Old rule: didn't ask for braces, new rule: asks for braces
> > > bar(1, 2,
> > > 3, 4);
> > > ```
> > > ```
> > > if (foo) // Old rule: didn't ask for braces, new rule: asks for braces
> > > bar(1, 2,
> > > 3, 4);
> > > else // Old rule: didn't ask for braces, new rule: doesn't ask for braces
> > > baz(1, 2);
> > > ```
> > > ```
> > > if (foo) // Old rule: didn't ask for braces, new rule: doesn't ask for braces
> > > baz(1, 2);
> > > else // Old rule: didn't ask for braces, new rule: asks for braces
> > > bar(1, 2,
> > > 3, 4);
> > > ```
> > > ```
> > > if (foo) // Old rule: didn't ask for braces, new rule: doesn't ask for braces
> > > if (bar) // Old rule: didn't ask for braces, new rule: doesn't ask for braces
> > > baz(1, 2);
> > > ```
> > > ```
> > > if (foo) // Old rule: didn't ask for braces, new rule: asks for braces
> > > if (bar(1, 2,
> > > 3, 4)) // Old rule: didn't ask for braces, new rule: doesn't ask for braces
> > > baz(1, 2);
> > > ```
> > > ```
> > > if (foo) // Old rule: didn't ask for braces, new rule: asks for braces
> > > for (int i = 0; i < 10; ++i) // Old rule: didn't ask for braces, new rule: asks for braces
> > > baz(i);
> > > ```
> > > Am I interpreting this correctly?
> > >
> > > (FWIW, I personally think it's better for us to err on recommending braces when they're unnecessary in these more complicated situations because that's the safer, more explicit style.)
> > Your examples are on point and are the reasons why I see this as a non-trivial policy change. At minima this requires a thread on discourse and a revision separate from the do/while case.
> >
> > ```
> > if (foo) // Old rule: didn't ask for braces, new rule: asks for braces
> > bar(1, 2,
> > 3, 4);
> > else // Old rule: didn't ask for braces, new rule: doesn't ask for braces
> > baz(1, 2);
> > ```
> The new rule asks for braces on the `if` (and because of the existing rule on 1636) and on the `else`.
> > ```
> > if (foo) // Old rule: didn't ask for braces, new rule: doesn't ask for braces
> > baz(1, 2);
> > else // Old rule: didn't ask for braces, new rule: asks for braces
> > bar(1, 2,
> > 3, 4);
> > ```
> Similarly, the new rule asks for braces on both `if` and `else`.
> > ```
> > if (foo) // Old rule: didn't ask for braces, new rule: doesn't ask for braces
> > if (bar) // Old rule: didn't ask for braces, new rule: doesn't ask for braces
> > baz(1, 2);
> > ```
> Both old and new rules ask for braces on the outer `if`, but the old example didn't really convey that.
> > ```
> > if (foo) // Old rule: didn't ask for braces, new rule: asks for braces
> > if (bar(1, 2,
> > 3, 4)) // Old rule: didn't ask for braces, new rule: doesn't ask for braces
> > baz(1, 2);
> > ```
> Both old and new rules ask braces on the outer `if`.
> > ```
> > if (foo) // Old rule: didn't ask for braces, new rule: asks for braces
> > for (int i = 0; i < 10; ++i) // Old rule: didn't ask for braces, new rule: asks for braces
> > baz(i);
> > ```
> Both old and new rules don't ask for **no** braces.
>
I agree that some subjectivity in some situations are a good thing, but "complex enough" is too vague to be useful here. I think this rule can be clear cut as it has already been implemented in clang-format. ;)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D126512/new/
https://reviews.llvm.org/D126512
More information about the llvm-commits
mailing list