[clang-tidy] `readability-redundant-parentheses` conflicts with suggested `-Wunreachable-code` mitigation

[firewave]

void f(bool b)
{
    if (false && b) {}
}

The code above will report the following:

<source>:3:18: warning: code will never be executed [-Wunreachable-code]
    3 |     if (false && b) {}
      |                  ^
<source>:3:9: note: silence by adding parentheses to mark code as explicitly dead
    3 |     if (false && b) {}
      |         ^
      |         /* DISABLES CODE */ ( )

But addressing that as suggested will lead to a clang-tidy warning:

<source>:3:9: warning: redundant parentheses around expression [readability-redundant-parentheses]
    3 |     if ((false) && b) {}
      |         ^     ~

https://godbolt.org/z/nTvEGrr7c

[llvmbot]

@llvm/issue-subscribers-clang-tidy

Author: Oliver Stöneberg (firewave)

```cpp void f(bool b) { if (false && b) {} } ```

The code above will report the following:

<source>:3:18: warning: code will never be executed [-Wunreachable-code]
    3 |     if (false && b) {}
      |                  ^
<source>:3:9: note: silence by adding parentheses to mark code as explicitly dead
    3 |     if (false && b) {}
      |         ^
      |         /* DISABLES CODE */ ( )

But addressing that as suggested will lead to a clang-tidy warning:

<source>:3:9: warning: redundant parentheses around expression [readability-redundant-parentheses]
    3 |     if ((false) && b) {}
      |         ^     ~

https://godbolt.org/z/nTvEGrr7c

[vbvictor]

I've seen this code in original PR:

if ((true) || !mSettings->severity.isEnabled(Severity::warning))

Just wondering, is there a reason to keep mSettings in if if the condition is always true?
Is this a popular pattern? If so, we could probably make an option to allow (true) and (false) expressions.

CC @HerrCai0907 as author.

[firewave]

Just wondering, is there a reason to keep mSettings in if if the condition is always true?

These are just temporary (unfortunately that is usually a few years in this case) hacks. There should be source level TODOs next to each of them. Some are also tracked in tickets.

Is this a popular pattern? If so, we could probably make an option to allow (true) and (false) expressions.

As far as hacks go I would consider it "common". Adding a bool literal in a condition instead of using #if 0 prevents bit-rot. Also it is a compiler suggested mitigation so it probably is more wide-spread now.

[carlosgalvezp]

If it's a temporary hack, I would rather have a NOLINT suppression that highlights the issue, than have the tools ignore it. Easier to grep for and audit/track too.

[HerrCai0907]

I am wonder where this hack come from. It is a bit counterintuitive.

[firewave]

If it's a temporary hack, I would rather have a NOLINT suppression that highlights the issue, than have the tools ignore it. Easier to grep for and audit/track too.

Make sense.

But what irks me is the flip-flopping and more ignorant users might just disable the warning/check in question because of that (and call it a "false positive").

[zufuliu]

if (false && b) {}

MSVC Code Analysis suggests using if constexpr (...) .