False positives when using misc-redundant-expression clang-tidy checker.

[earnol]

The misc-redundant-expression produces a warning with latest trunk build where it should not.
In the example below two different defines are being compared. Those defines different, yet happens to have the same value due to compile defines configuration.
The warning generated is:

<source>:13:20: warning: both sides of operator are equivalent [misc-redundant-expression]
   13 |   if (VAL_1 != VAL_2)

where operator != is highlighted.
I believe the programmer intention is very clear here as programmer wants to have function foo return 0 or 1 depending on the define passed into the compilation line, so the condition is not redundant.

I have created a minimal reproducer for the issue:

//#define A
#ifndef A
#define VAL_1 2
#else
#define VAL_1 3
#endif

#define VAL_2 2

unsigned foo() {
  if (VAL_1 != VAL_2)
  {
    return 0;
  }
  else
  {
    return 1;
  }
}


int main() {
  return foo();
}

The example is also available on the godbolt: https://godbolt.org/z/3sd6qTfTP.

It look like a bug for me because in file clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp in function
void RedundantExpressionCheck::check(const MatchFinder::MatchResult &Result)
the check areExprsFromDifferentMacros(LhsConst, RhsConst, Result.Context) is called explicitly to prevent exactly this false positive.
Here is the link to the source:

llvm-project/clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp

Line 1287 in a9aff44

if (areExprsFromDifferentMacros(LhsConst, RhsConst, Result.Context) ||

[mzyKi]

I do not think this is a false positive. if A is defined,VAL_1 is 3, and there will be no report about misc-redundant-expression

[earnol]

You are right. There will be no diagnostics in this case, but it will make expression on line 12 redundant the exactly same way. The only difference it will be always TRUE, instead of being always FALSE. So for the sake of logical consistency it should be either always redundant or never be the one.
Moreover there is a logic in the code 1287 which specifically has intent to prevent diagnostics making it "never". Thus it still looks for me as a false positive.

[vabridgers]

What's the suggested "fix" for this issue? I suppose it's possible for 2 different defined macros to have the same numerical value but used in different meanings.

The check could be weakened to avoid report reporting issues like this and only detect these issues when a "pedantic" option is used (set to false by default and would need to be explicitly set to true).

The other way is to try checking the macro names to see if "dissimilar" or "similar" in some way - that seems like not a good idea because of no good way to define "similar" and/or "dissimilar".

Another way - just weaken the check to avoid reporting these cases.

Finally, maybe just keep the check this way. The user would be required to suppress cases like this.

Thoughts?

[earnol]

My understanding is there should be a consistent logic:

1. We report all situations when when expression consisting of literals is compile time are defined. Like if (5 != 6) regardless whether it literals, constants or defines, but not expressions like if (5 != (4 + 2)) when the one or two expression arguments are expression themselves. This is not a current behavior and making hidden under pedantic option might be useful sometimes.
2. We report only situations where where the same lexical element is used inside the expression. For example:
   if ((5 + 1) != (5 + 1)) or if (5 == 5) or if (A_DEFINE == A_DEFINE) or if (const_variable != const_variable) or if ((A_DEFINE - 1) != (A_DEFINE - 1)). I think this is a desirable default behavior for this checker.
   The current behavior where we report TRUE condition and not reporting FALSE condition is the way too confusing and thus does not look acceptable.

[vabridgers]

There needs to be a clear definition of what constitutes a "redundant expression". In this case, who's to say that the values of VAL_1 and VAL_2 may change in future (if initially different), and the user wants to detect if the values change in an unexpected way.

[5chmidti]

It look like a bug for me because in file clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp in function
void RedundantExpressionCheck::check(const MatchFinder::MatchResult &Result)
the check areExprsFromDifferentMacros(LhsConst, RhsConst, Result.Context) is called explicitly to prevent exactly this false positive.

That check is inside an if statement predicated on areSidesBinaryConstExpressions, which requires both sides of a binary operator to be binary operators as well (+ some more), so the code in the example never takes that branch. If you add + 1 to each of the defined macros, the warning is not issued because of the check you've mentioned: https://godbolt.org/z/csaqhaMzG

I do not think this is a false positive. if A is defined,VAL_1 is 3, and there will be no report about misc-redundant-expression

But the macros can have fully disjoint purposes, they just happen to have the same value and be useful to compare.

The check could be weakened to avoid reporting issues like this, and only detect these issues when a "pedantic" option is used (set to false by default and would need to be explicitly set to true).

I think that is the right solution for the current state of the check, which should also be implemented for the areExprsFromDifferentMacros condition in the original post.

There needs to be a clear definition of what constitutes a "redundant expression". In this case, who's to say that the values of VAL_1 and VAL_2 may change in future (if initially different)

• We report all situations when when expression consisting of literals is compile time are defined. Like if (5 != 6) regardless whether it literals, constants or defines, but not expressions like if (5 != (4 + 2)) when the one or two expression arguments are expression themselves. This is not a current behavior and making hidden under pedantic option might be useful sometimes.
• We report only situations where where the same lexical element is used inside the expression. For example:
  if ((5 + 1) != (5 + 1)) or if (5 == 5) or if (A_DEFINE == A_DEFINE) or if (const_variable != const_variable) or if ((A_DEFINE - 1) != (A_DEFINE - 1)). I think this is a desirable default behavior for this checker.
  The current behavior where we report TRUE condition and not reporting FALSE condition is the way too confusing and thus does not look acceptable.

It'd be interesting to think about having both kinds of redundant expressions be checked. But as you stated, I think this makes only sense for literals. E.g., N + 1 == N + 2 - 1

who's to say that the values of VAL_1 and VAL_2 may change in future (if initially different), and the user wants to detect if the values change in an unexpected way.

That sounds like a job for tests.

---

(re-tagged due to missing bot ping)

[llvmbot]

@llvm/issue-subscribers-clang-tidy

Author: None (earnol)

The misc-redundant-expression produces a warning with latest trunk build where it should not. In the example below two different defines are being compared. Those defines different, yet happens to have the same value due to compile defines configuration. The warning generated is: ``` <source>:13:20: warning: both sides of operator are equivalent [misc-redundant-expression] 13 | if (VAL_1 != VAL_2) ``` where operator != is highlighted. I believe the programmer intention is very clear here as programmer wants to have function foo return 0 or 1 depending on the define passed into the compilation line, so the condition is not redundant.

I have created a minimal reproducer for the issue:

//#define A
#ifndef A
#define VAL_1 2
#else
#define VAL_1 3
#endif

#define VAL_2 2

unsigned foo() {
  if (VAL_1 != VAL_2)
  {
    return 0;
  }
  else
  {
    return 1;
  }
}


int main() {
  return foo();
}

The example is also available on the godbolt: https://godbolt.org/z/3sd6qTfTP.

It look like a bug for me because in file clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp in function
void RedundantExpressionCheck::check(const MatchFinder::MatchResult &Result)
the check areExprsFromDifferentMacros(LhsConst, RhsConst, Result.Context) is called explicitly to prevent exactly this false positive.
Here is the link to the source:

llvm-project/clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp

Line 1287 in a9aff44

if (areExprsFromDifferentMacros(LhsConst, RhsConst, Result.Context) ||

[earnol]

That check is inside an if statement predicated on areSidesBinaryConstExpressions, which requires both sides of a binary operator to be binary operators as well (+ some more), so the code in the example never takes that branch. If you add + 1 to each of the defined macros, the warning is not issued because of the check you've mentioned: https://godbolt.org/z/csaqhaMzG

You are correct. But i do not believe adding a clutch like + 1 to each of the defined macros is a good solution. It actually looks pretty ugly. So this problem should be fixed.

But the macros can have fully disjoint purposes, they just happen to have the same value and be useful to compare.

The key is we (as compiler developers) do not know and it is up to the user to define. But what makes define it as a false positive is that macro comparison is a totally valid development practice on C/C++ in many areas.

I think that is the right solution for the current state of the check, which should also be implemented for the areExprsFromDifferentMacros condition in the original post.

Yeap. This function was "enhanced" i would say.

It'd be interesting to think about having both kinds of redundant expressions be checked. But as you stated, I think this makes only sense for literals. E.g., N + 1 == N + 2 - 1

In this situation i would say it is not redundant. The developer can be lazy to calculate expressions (especially if those are 5 digits numbers) but this does not look like a typo or copy/paste bug. My understanding of clang-tidy as a tool that detects(mostly) silly copy/paste, operator precedence, formatting style and similar bugs and putting literally (as string literals) different expressions definitely is not falling into this list.

That sounds like a job for tests.

Yeap. Added a lot of new tests to #121960 partially addressing this issue.

PS: Not closing as patch does NOT address all concerns here besides an initial report.