1602143 - Make clang-tidy readability-braces-around-statements less verbose

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, enhancement, P3)

Description

[Kelsey Gilbert [:jgilbert]]

Until clang-format injects these modifications, we shouldn't complain so loudly about this.

Reasoning from bug 1558987 applies as well.

FWIW I'm against this warning in general. readability-misleading-indentation is the warning that catches the classic "goto FAIL;" bug, and we already have that. This lint pass however subjectively requires extra visual noise for simple branches like if (error) continue;.

Comment 1

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1602143 - Mute clang-tidy readability-braces-around-statements.

Comment 2

[Kelsey Gilbert [:jgilbert]]

Here's an example where this warning is overly noisy:
https://phabricator.services.mozilla.com/D54019#1710249

Comment 3

[Sylvestre Ledru [:Sylvestre]]

As this is part of our coding style, I don't think we should disable this warning.
https://firefox-source-docs.mozilla.org/tools/lint/coding-style/coding_style_cpp.html#control-structures

Moreover, it can easily be fixed with:

 ./mach static-analysis check --checks="-*, google-readability-braces-around-statements" --fix dom/canvas/WebGLContext.cpp

I would be incline to mark this bug as WONTFIX but I will let Ehsan make the call.

Comment 4

[Kelsey Gilbert [:jgilbert]]

The stated reasoning there is "This is redundant, typically, but it avoids dangling else bugs, so it’s safer at scale than fine-tuning.", which no longer applies, now that we run clang-tidy. Now that this recommendation is just "This is redundant, typically", I suppose I'll propose removing the recommendation.

Comment 5

[Kelsey Gilbert [:jgilbert]]

I do want to be clear, and want you to understand though, that these warnings are not convincing me to add braces, they have instead convinced me to ignore the high-noise lint checks that are supposed to be helpful for me.

The goal for these lint checks should be to prevent bugs, and this doesn't prevent bugs. It's a style-nit, which, as part of the pivot to clang-format, was purported to be a benefit of autoformatting: No more code review nits for style fixes! However, today, we have them, they're just automated now.

This lint pass does not add value, and is making me do busywork. Please remove it.

Comment 6

[Sylvestre Ledru [:Sylvestre]]

Yeah, this is one of my worries indeed.
A solution could be to reformat the tree to add braces everywhere.

Comment 7

[Kelsey Gilbert [:jgilbert]]

Is your reasoning for keeping the braces entirely because that's what the style guide, as I quoted, says?

I'll note that the quickest path to making the most people happy here is to mute this lint warning.

Comment 8

[Kelsey Gilbert [:jgilbert]]

I filed bug 1602248 about removing this obsolete style recommendation.

Comment 9

[(no longer active)]

While I disagree that we want to go as far as bug 1602248, I think I agree with Jeff that a) this warning can get pretty noisy for code that doesn't adhere to it (which is something that is permitted by the Google C++ style) and b) people are very unlikely to respond to it in a favourable fashion (IOW by changing their code to abide by it) since this is usually a very deliberate decision in writing code, especially when the warning gets to the levels of it being noisy.

I am inclined to turn the warning off by default, given that those who would prefer the braced style can easily get it by running a command. Perhaps we should also document that command in the future as well.

In general I would like the review bot to only make comments about things that developers want to act on. Developers should want to read its output, and should feel incentivized to act on it. If we don't respond to complaints like this we run the risk of creating adversarial relationships against the reviewbot, which will serve nobody in the end IMO.

Comment 10

[Sylvestre Ledru [:Sylvestre]]

A suggestion to mitigate the concern, we should limit the display of this warning to one occurence and update the warning.
We could say "Some braces are missing in this file. you can fix that automatically by running ./mach static-..."

We could also reuse this feature for other verbose checkers.

wdyt?

Comment 11

[(no longer active)]

(In reply to Sylvestre Ledru [:Sylvestre] from comment #10)

A suggestion to mitigate the concern, we should limit the display of this warning to one occurence and update the warning.
We could say "Some braces are missing in this file. you can fix that automatically by running ./mach static-..."

We could also reuse this feature for other verbose checkers.

wdyt?

Sure if that's easy. I don't really have any strong opinions on how we achieve the goal. Jeff, what do you think?

Comment 12

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1602143 - Use ShortStatementLines=1 for readability-braces-around-statements.

This will reduce the false-positive rate that sometimes causes devs to
ignore other important warnings because they expect to see these
warnings because of module/legacy code styles.

if (foo) return; // no longer warns
if (bar)
  return; // still warns

clang-format will sometimes turn long-line versions of the former into
the latter, and this warning will pick those as readability warnings.
NB: This is Chromium style for same-line conditional statements.

Another test result json has its EOL-newline removed too, since that's
what the script does and I'm just committing its results.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0ef5ba276608
Use ShortStatementLines=1 for readability-braces-around-statements. r=andi

Comment 14

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/0ef5ba276608