[clang-tidy] New check: bugprone-ifelse-braces

[capitan-davide]

A simple check that warns, and potentially fixes, the use of unmatched compound statements (i.e., braces) following an if/else statements. For example:

void f(bool Flag) {
  if (Flag)
    doSomething();
  else {
    // we need to do something else here!
    doSomethingElse();
  }
}

This example will trigger a warning since the if branch do not uses braces, while the else branch does. Vice-versa also applies.

[llvmbot]

@llvm/issue-subscribers-clang-tidy

Author: Davide Cunial (capitan-davide)

A simple check that warns, and potentially fixes, the use of unmatched compound statements (i.e., braces) following an if/else statements. For example:

void f(bool Flag) {
  if (Flag)
    doSomething();
  else {
    // we need to do something else here!
    doSomethingElse();
  }
}

This example will trigger a warning since the if branch do not uses braces, while the else branch does. Vice-versa also applies.

[EugeneZelenko]

Should readability-braces-around-statements be extended?

[LegalizeAdulthood]

Should readability-braces-around-statements be extended?

The check already detects this case: https://godbolt.org/z/dvbnr6vzT

[vbvictor]

Maybe this check want behavior in https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements:

readability is also harmed if an if/else chain does not use braced bodies for either all or none of its members

So It's either all braces or none braces.

[LegalizeAdulthood]

braces should be used in cases where the omission of braces harms the readability and maintainability of the code.

Honestly, this is case of the "guidelines" going too far, IMO. My reason for saying this is that now you're introducing subjectivity into the guidelines and the guideline authors have their point of view and every programmer in the universe has their (possibly divergent) point of view. Guidelines should be universal, not subjective.

The CERT C guidelines require that braces always be used. The reasoning being that omitting braces can cause subtle errors as the code is edited.

My personal formatting style is that braces go on a line by themselves and braces are always required on control statements. I intentionally want braces to chew up vertical whitespace to discourage long functions. Therefore I conclude that omitting braces "harms the readability and maintainability of the code" in ALL cases, not just some subjectively selected cases.

Even worse, this guideline being subjective means that there is no mechanism for applying automated tooling to ensure compliance with the guideline.

[capitan-davide]

I agree with @vbvictor. My point for introducing this new check is to have a less opinionated check for what "readability" means. The aim is to address only the "bugprone" part i.e., being able to identify which else binds to which if.

This is less restrictive than 'readability-braces-around-statements', just checking for uniformity of braces (requested by the LLVM coding style). The 'readability-braces-around-statements' check contradicts the LLVM coding style and could not be enabled for the project.

[LegalizeAdulthood]

My point for introducing this new check is to have a less opinionated check for what "readability" means.

So it's the 'all braces or no braces' part that this new check is intended to enforce? How about the name bugprone-inconsistent-brace-usage or the simpler bugprone-inconsistent-braces then?

[capitan-davide]

My point for introducing this new check is to have a less opinionated check for what "readability" means.

So it's the 'all braces or no braces' part that this new check is intended to enforce? How about the name bugprone-inconsistent-brace-usage or the simpler bugprone-inconsistent-braces then?

Yes, both 'all braces or no braces'. The idea is to analyze every ifStmt separetely and, if any branch has braces then all branches should have braces.

About the naming, no strong opinions. I just made up a quick one that I am not completely happy with :) (but maybe it should communicate that this applies only to if statements somehow, or?)

What about bugprone-inconsistent-ifelse-braces?

[LegalizeAdulthood]

What about bugprone-inconsistent-ifelse-braces?

I guess this only comes up for the if case and not the other control structures, so that is a more precise name.

[capitan-davide]

When I was looking into offering fixes for this check, I noticed that 'readability-braces-around-statements' does not offer fixes either.

Should we add fixes for 'readability-braces-around-statements' as well? What do you think? @vbvictor @EugeneZelenko @LegalizeAdulthood

[LegalizeAdulthood]

I noticed that 'readability-braces-around-statements' does not offer fixes either.

Are you sure? The compiler explorer link I posted earlier on this discussion showed that it does for at least the case in this bug.

[capitan-davide]

I noticed that 'readability-braces-around-statements' does not offer fixes either.

Are you sure? The compiler explorer link I posted earlier on this discussion showed that it does for at least the case in this bug.

Ah you're right! I guess the documentation is not up-to date. #162698

[capitan-davide]

In the linked PR, you can find the proposed implementation. While it's still draft, the business logic is there. Only the tests needs to be expanded.

For example:

// input.cc
if (cond("if0") /*comment*/) do_something("if-same-line");
else {}

Emits the following warning:

input.cc:2:31: warning: <message> [bugprone-inconsistent-ifelse-braces]
    2 |   if (cond("if0") /*comment*/) do_something("if-same-line");
      |                               ^
      |                                {
    3 |   else {}
      |   }