Skip to content

[clang-tidy] Address false positives in misc-redundant-expression checker #122841

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Feb 10, 2025
Merged

[clang-tidy] Address false positives in misc-redundant-expression checker #122841

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c26434f
[clang-tidy] Address false positives in misc-redundant-expression che…
Jan 14, 2025
8f7dd8b
Update clang-tools-extra/docs/ReleaseNotes.rst
earnol Jan 27, 2025
69c39bf
Merge branch 'main' into main
earnol Jan 30, 2025
e4bb607
Empty line separator added
earnol Jan 31, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
145 changes: 121 additions & 24 deletions clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,10 +139,8 @@ static bool areEquivalentExpr(const Expr *Left, const Expr *Right) {
return cast<BinaryOperator>(Left)->getOpcode() ==
cast<BinaryOperator>(Right)->getOpcode();
case Stmt::UnaryExprOrTypeTraitExprClass:
const auto *LeftUnaryExpr =
cast<UnaryExprOrTypeTraitExpr>(Left);
const auto *RightUnaryExpr =
cast<UnaryExprOrTypeTraitExpr>(Right);
const auto *LeftUnaryExpr = cast<UnaryExprOrTypeTraitExpr>(Left);
const auto *RightUnaryExpr = cast<UnaryExprOrTypeTraitExpr>(Right);
if (LeftUnaryExpr->isArgumentType() && RightUnaryExpr->isArgumentType())
return LeftUnaryExpr->getKind() == RightUnaryExpr->getKind() &&
LeftUnaryExpr->getArgumentType() ==
Expand Down Expand Up @@ -699,7 +697,8 @@ static bool retrieveRelationalIntegerConstantExpr(

Symbol = OverloadedOperatorExpr->getArg(IntegerConstantIsFirstArg ? 1 : 0);
OperandExpr = OverloadedOperatorExpr;
Opcode = BinaryOperator::getOverloadedOpcode(OverloadedOperatorExpr->getOperator());
Opcode = BinaryOperator::getOverloadedOpcode(
OverloadedOperatorExpr->getOperator());

if (!retrieveIntegerConstantExpr(Result, Id, Value, ConstExpr))
return false;
Expand Down Expand Up @@ -728,7 +727,8 @@ static bool retrieveRelationalIntegerConstantExpr(
}

// Checks for expressions like (X == 4) && (Y != 9)
static bool areSidesBinaryConstExpressions(const BinaryOperator *&BinOp, const ASTContext *AstCtx) {
static bool areSidesBinaryConstExpressions(const BinaryOperator *&BinOp,
const ASTContext *AstCtx) {
const auto *LhsBinOp = dyn_cast<BinaryOperator>(BinOp->getLHS());
const auto *RhsBinOp = dyn_cast<BinaryOperator>(BinOp->getRHS());

Expand All @@ -747,6 +747,28 @@ static bool areSidesBinaryConstExpressions(const BinaryOperator *&BinOp, const A
return false;
}

static bool areSidesBinaryConstExpressionsOrDefinesOrIntegerConstant(
const BinaryOperator *&BinOp, const ASTContext *AstCtx) {
if (areSidesBinaryConstExpressions(BinOp, AstCtx))
return true;

const Expr *Lhs = BinOp->getLHS();
const Expr *Rhs = BinOp->getRHS();

if (!Lhs || !Rhs)
return false;

auto IsDefineExpr = [AstCtx](const Expr *E) {
const SourceRange Lsr = E->getSourceRange();
if (!Lsr.getBegin().isMacroID() || E->isValueDependent() ||
!E->isIntegerConstantExpr(*AstCtx))
return false;
return true;
};

return IsDefineExpr(Lhs) || IsDefineExpr(Rhs);
}

// Retrieves integer constant subexpressions from binary operator expressions
// that have two equivalent sides.
// E.g.: from (X == 5) && (X == 5) retrieves 5 and 5.
Expand Down Expand Up @@ -785,7 +807,7 @@ static bool retrieveConstExprFromBothSides(const BinaryOperator *&BinOp,
}

static bool isSameRawIdentifierToken(const Token &T1, const Token &T2,
const SourceManager &SM) {
const SourceManager &SM) {
if (T1.getKind() != T2.getKind())
return false;
if (T1.isNot(tok::raw_identifier))
Expand All @@ -808,8 +830,8 @@ static bool areExprsFromDifferentMacros(const Expr *LhsExpr,
const ASTContext *AstCtx) {
if (!LhsExpr || !RhsExpr)
return false;
SourceRange Lsr = LhsExpr->getSourceRange();
SourceRange Rsr = RhsExpr->getSourceRange();
const SourceRange Lsr = LhsExpr->getSourceRange();
const SourceRange Rsr = RhsExpr->getSourceRange();
if (!Lsr.getBegin().isMacroID() || !Rsr.getBegin().isMacroID())
return false;

Expand Down Expand Up @@ -847,11 +869,83 @@ static bool areExprsMacroAndNonMacro(const Expr *&LhsExpr,
if (!LhsExpr || !RhsExpr)
return false;

SourceLocation LhsLoc = LhsExpr->getExprLoc();
SourceLocation RhsLoc = RhsExpr->getExprLoc();
const SourceLocation LhsLoc = LhsExpr->getExprLoc();
const SourceLocation RhsLoc = RhsExpr->getExprLoc();

return LhsLoc.isMacroID() != RhsLoc.isMacroID();
}

static bool areStringsSameIgnoreSpaces(const llvm::StringRef Left,
const llvm::StringRef Right) {
if (Left == Right)
return true;

// Do running comparison ignoring spaces
llvm::StringRef L = Left.trim();
llvm::StringRef R = Right.trim();
while (!L.empty() && !R.empty()) {
L = L.ltrim();
R = R.ltrim();
if (L.empty() && R.empty())
return true;
// If symbol compared are different ==> strings are not the same
if (L.front() != R.front())
return false;
L = L.drop_front();
R = R.drop_front();
}
return L.empty() && R.empty();
}

static bool areExprsSameMacroOrLiteral(const BinaryOperator *BinOp,
const ASTContext *Context) {

if (!BinOp)
return false;

const Expr *Lhs = BinOp->getLHS();
const Expr *Rhs = BinOp->getRHS();
const SourceManager &SM = Context->getSourceManager();

const SourceRange Lsr = Lhs->getSourceRange();
const SourceRange Rsr = Rhs->getSourceRange();
if (Lsr.getBegin().isMacroID()) {
Copy link
Member

@PiotrZSL PiotrZSL Jan 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

problem with this is that you do not know if every part of LHS or RHS is an macro, because you checking only source of first token. If I would write: +VALUE, where #define VALUE 10, then it would fail.

I think that there is some function in /utils/ that may help here

Copy link
Contributor Author

@earnol earnol Jan 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking expressions like +VALUE was not a goal of this function in the first place.
But i agree the case like if(+VALUE == +VALUE) is redundant and should be properly detected. I'll add it to lit tests and make sure it will get detected.

Copy link
Contributor Author

@earnol earnol Jan 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added extra lit tests to cover this situation. Please check it out.

// Left is macro so right macro too
if (Rsr.getBegin().isMacroID()) {
// Both sides are macros so they are same macro or literal
const llvm::StringRef L = Lexer::getSourceText(
CharSourceRange::getTokenRange(Lsr), SM, Context->getLangOpts(), 0);
const llvm::StringRef R = Lexer::getSourceText(
CharSourceRange::getTokenRange(Rsr), SM, Context->getLangOpts(), 0);
return areStringsSameIgnoreSpaces(L, R);
}
// Left is macro but right is not so they are not same macro or literal
return false;
}
const auto *Lil = dyn_cast<IntegerLiteral>(Lhs);
const auto *Ril = dyn_cast<IntegerLiteral>(Rhs);
if (Lil && Ril)
return Lil->getValue() == Ril->getValue();

const auto *LStrl = dyn_cast<StringLiteral>(Lhs);
const auto *RStrl = dyn_cast<StringLiteral>(Rhs);
if (Lil && Ril) {
const llvm::StringRef L = Lexer::getSourceText(
CharSourceRange::getTokenRange(LStrl->getBeginLoc()), SM,
Context->getLangOpts(), 0);
const llvm::StringRef R = Lexer::getSourceText(
CharSourceRange::getTokenRange(RStrl->getBeginLoc()), SM,
Context->getLangOpts(), 0);
return L.compare(R) == 0;
}

const auto *Lbl = dyn_cast<CXXBoolLiteralExpr>(Lhs);
const auto *Rbl = dyn_cast<CXXBoolLiteralExpr>(Rhs);
if (Lbl && Rbl)
return Lbl->getValue() == Rbl->getValue();

return false;
}
} // namespace

void RedundantExpressionCheck::registerMatchers(MatchFinder *Finder) {
Expand Down Expand Up @@ -1089,7 +1183,6 @@ static bool exprEvaluatesToSymbolic(BinaryOperatorKind Opcode, APSInt Value) {
((Opcode == BO_And || Opcode == BO_AndAssign) && ~Value == 0);
}


void RedundantExpressionCheck::checkBitwiseExpr(
const MatchFinder::MatchResult &Result) {
if (const auto *ComparisonOperator = Result.Nodes.getNodeAs<BinaryOperator>(
Expand Down Expand Up @@ -1134,8 +1227,8 @@ void RedundantExpressionCheck::checkBitwiseExpr(
ConstExpr))
return;

if((Value != 0 && ~Value != 0) || Sym->getExprLoc().isMacroID())
return;
if ((Value != 0 && ~Value != 0) || Sym->getExprLoc().isMacroID())
return;

SourceLocation Loc = IneffectiveOperator->getOperatorLoc();

Expand Down Expand Up @@ -1276,19 +1369,23 @@ void RedundantExpressionCheck::check(const MatchFinder::MatchResult &Result) {
return;
}

if (areSidesBinaryConstExpressions(BinOp, Result.Context)) {
if (areSidesBinaryConstExpressionsOrDefinesOrIntegerConstant(
BinOp, Result.Context)) {
const Expr *LhsConst = nullptr, *RhsConst = nullptr;
BinaryOperatorKind MainOpcode{}, SideOpcode{};

if (!retrieveConstExprFromBothSides(BinOp, MainOpcode, SideOpcode,
LhsConst, RhsConst, Result.Context))
return;

if (areExprsFromDifferentMacros(LhsConst, RhsConst, Result.Context) ||
areExprsMacroAndNonMacro(LhsConst, RhsConst))
return;
if (areSidesBinaryConstExpressions(BinOp, Result.Context)) {
if (!retrieveConstExprFromBothSides(BinOp, MainOpcode, SideOpcode,
LhsConst, RhsConst, Result.Context))
return;

if (areExprsFromDifferentMacros(LhsConst, RhsConst, Result.Context) ||
areExprsMacroAndNonMacro(LhsConst, RhsConst))
return;
} else {
if (!areExprsSameMacroOrLiteral(BinOp, Result.Context))
return;
}
}

diag(BinOp->getOperatorLoc(), "both sides of operator are equivalent");
}

Expand Down
3 changes: 3 additions & 0 deletions clang-tools-extra/docs/ReleaseNotes.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,9 @@ Changes in existing checks
- Improved :doc:`bugprone-unsafe-functions
<clang-tidy/checks/bugprone/unsafe-functions>` check to allow specifying
additional C++ member functions to match.
- Improved :doc:`misc-redundant-expression
<clang-tidy/checks/misc/redundant-expression>` check by providing additional
examples and fixing some macro related false positives.

Removed checks
^^^^^^^^^^^^^^
Expand Down
14 changes: 8 additions & 6 deletions clang-tools-extra/docs/clang-tidy/checks/misc/redundant-expression.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,14 @@ Examples:

.. code-block:: c++

((x+1) | (x+1)) // (x+1) is redundant
(p->x == p->x) // always true
(p->x < p->x) // always false
(speed - speed + 1 == 12) // speed - speed is always zero
int b = a | 4 | a // identical expr on both sides
((x=1) | (x=1)) // expression is identical
((x+1) | (x+1)) // (x+1) is redundant
(p->x == p->x) // always true
(p->x < p->x) // always false
(speed - speed + 1 == 12) // speed - speed is always zero
int b = a | 4 | a // identical expr on both sides
((x=1) | (x=1)) // expression is identical
(DEFINE_1 | DEFINE_1) // same macro on the both sides
((DEF_1 + DEF_2) | (DEF_1+DEF_2)) // expressions differ in spaces only

Floats are handled except in the case that NaNs are checked like so:

Expand Down
Loading