[clang-tools-extra] 5e7d0eb - Cover cases like (b && c && b) in the redundant expression check.
Aaron Ballman via cfe-commits
cfe-commits at lists.llvm.org
Tue Feb 18 08:44:41 PST 2020
Author: Alexey Romanov
Date: 2020-02-18T11:42:32-05:00
New Revision: 5e7d0ebf735a8b70f92acd1f91c7c45423e611cc
URL: https://github.com/llvm/llvm-project/commit/5e7d0ebf735a8b70f92acd1f91c7c45423e611cc
DIFF: https://github.com/llvm/llvm-project/commit/5e7d0ebf735a8b70f92acd1f91c7c45423e611cc.diff
LOG: Cover cases like (b && c && b) in the redundant expression check.
readability-redundant-expression now detects expressions where a logical
or bitwise operator had equivalent LHS and RHS where the equivalent
operands were separated by more operands.
Added:
Modified:
clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp b/clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
index e3e84b71601b..90fcf38f83b7 100644
--- a/clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
+++ b/clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
@@ -18,7 +18,9 @@
#include "llvm/ADT/APInt.h"
#include "llvm/ADT/APSInt.h"
#include "llvm/ADT/FoldingSet.h"
+#include "llvm/ADT/SmallBitVector.h"
#include "llvm/Support/Casting.h"
+#include "llvm/Support/FormatVariadic.h"
#include <algorithm>
#include <cassert>
#include <cstdint>
@@ -304,6 +306,132 @@ static void transformSubToCanonicalAddExpr(BinaryOperatorKind &Opcode,
}
}
+// to use in the template below
+static OverloadedOperatorKind getOp(const BinaryOperator *Op) {
+ return BinaryOperator::getOverloadedOperator(Op->getOpcode());
+}
+
+static OverloadedOperatorKind getOp(const CXXOperatorCallExpr *Op) {
+ if (Op->getNumArgs() != 2)
+ return OO_None;
+ return Op->getOperator();
+}
+
+static std::pair<const Expr *, const Expr *>
+getOperands(const BinaryOperator *Op) {
+ return {Op->getLHS()->IgnoreParenImpCasts(),
+ Op->getRHS()->IgnoreParenImpCasts()};
+}
+
+static std::pair<const Expr *, const Expr *>
+getOperands(const CXXOperatorCallExpr *Op) {
+ return {Op->getArg(0)->IgnoreParenImpCasts(),
+ Op->getArg(1)->IgnoreParenImpCasts()};
+}
+
+template <typename TExpr>
+static const TExpr *checkOpKind(const Expr *TheExpr,
+ OverloadedOperatorKind OpKind) {
+ const auto *AsTExpr = dyn_cast_or_null<TExpr>(TheExpr);
+ if (AsTExpr && getOp(AsTExpr) == OpKind)
+ return AsTExpr;
+
+ return nullptr;
+}
+
+// returns true if a subexpression has two directly equivalent operands and
+// is already handled by operands/parametersAreEquivalent
+template <typename TExpr, unsigned N>
+static bool collectOperands(const Expr *Part,
+ SmallVector<const Expr *, N> &AllOperands,
+ OverloadedOperatorKind OpKind) {
+ if (const auto *BinOp = checkOpKind<TExpr>(Part, OpKind)) {
+ const std::pair<const Expr *, const Expr *> Operands = getOperands(BinOp);
+ if (areEquivalentExpr(Operands.first, Operands.second))
+ return true;
+ return collectOperands<TExpr>(Operands.first, AllOperands, OpKind) ||
+ collectOperands<TExpr>(Operands.second, AllOperands, OpKind);
+ }
+
+ AllOperands.push_back(Part);
+ return false;
+}
+
+template <typename TExpr>
+static bool hasSameOperatorParent(const Expr *TheExpr,
+ OverloadedOperatorKind OpKind,
+ ASTContext &Context) {
+ // IgnoreParenImpCasts logic in reverse: skip surrounding uninteresting nodes
+ const DynTypedNodeList Parents = Context.getParents(*TheExpr);
+ for (ast_type_traits::DynTypedNode DynParent : Parents) {
+ if (const auto *Parent = DynParent.get<Expr>()) {
+ bool Skip = isa<ParenExpr>(Parent) || isa<ImplicitCastExpr>(Parent) ||
+ isa<FullExpr>(Parent) ||
+ isa<MaterializeTemporaryExpr>(Parent);
+ if (Skip && hasSameOperatorParent<TExpr>(Parent, OpKind, Context))
+ return true;
+ if (checkOpKind<TExpr>(Parent, OpKind))
+ return true;
+ }
+ }
+
+ return false;
+}
+
+template <typename TExpr>
+static bool
+markDuplicateOperands(const TExpr *TheExpr,
+ ast_matchers::internal::BoundNodesTreeBuilder *Builder,
+ ASTContext &Context) {
+ const OverloadedOperatorKind OpKind = getOp(TheExpr);
+ if (OpKind == OO_None)
+ return false;
+ // if there are no nested operators of the same kind, it's handled by
+ // operands/parametersAreEquivalent
+ const std::pair<const Expr *, const Expr *> Operands = getOperands(TheExpr);
+ if (!(checkOpKind<TExpr>(Operands.first, OpKind) ||
+ checkOpKind<TExpr>(Operands.second, OpKind)))
+ return false;
+
+ // if parent is the same kind of operator, it's handled by a previous call to
+ // markDuplicateOperands
+ if (hasSameOperatorParent<TExpr>(TheExpr, OpKind, Context))
+ return false;
+
+ SmallVector<const Expr *, 4> AllOperands;
+ if (collectOperands<TExpr>(Operands.first, AllOperands, OpKind))
+ return false;
+ if (collectOperands<TExpr>(Operands.second, AllOperands, OpKind))
+ return false;
+ size_t NumOperands = AllOperands.size();
+ llvm::SmallBitVector Duplicates(NumOperands);
+ for (size_t I = 0; I < NumOperands; I++) {
+ if (Duplicates[I])
+ continue;
+ bool FoundDuplicates = false;
+
+ for (size_t J = I + 1; J < NumOperands; J++) {
+ if (AllOperands[J]->HasSideEffects(Context))
+ break;
+
+ if (areEquivalentExpr(AllOperands[I], AllOperands[J])) {
+ FoundDuplicates = true;
+ Duplicates.set(J);
+ Builder->setBinding(
+ SmallString<11>(llvm::formatv("duplicate{0}", J)),
+ ast_type_traits::DynTypedNode::create(*AllOperands[J]));
+ }
+ }
+
+ if (FoundDuplicates)
+ Builder->setBinding(
+ SmallString<11>(llvm::formatv("duplicate{0}", I)),
+ ast_type_traits::DynTypedNode::create(*AllOperands[I]));
+ }
+
+ return Duplicates.any();
+}
+
AST_MATCHER(Expr, isIntegerConstantExpr) {
if (Node.isInstantiationDependent())
return false;
@@ -314,6 +442,10 @@ AST_MATCHER(BinaryOperator, operandsAreEquivalent) {
return areEquivalentExpr(Node.getLHS(), Node.getRHS());
}
+AST_MATCHER(BinaryOperator, nestedOperandsAreEquivalent) {
+ return markDuplicateOperands(&Node, Builder, Finder->getASTContext());
+}
+
AST_MATCHER(ConditionalOperator, expressionsAreEquivalent) {
return areEquivalentExpr(Node.getTrueExpr(), Node.getFalseExpr());
}
@@ -323,6 +455,10 @@ AST_MATCHER(CallExpr, parametersAreEquivalent) {
areEquivalentExpr(Node.getArg(0), Node.getArg(1));
}
+AST_MATCHER(CXXOperatorCallExpr, nestedParametersAreEquivalent) {
+ return markDuplicateOperands(&Node, Builder, Finder->getASTContext());
+}
+
AST_MATCHER(BinaryOperator, binaryOperatorIsInMacro) {
return Node.getOperatorLoc().isMacroID();
}
@@ -484,8 +620,15 @@ static bool isNonConstReferenceType(QualType ParamType) {
// is a temporary expression. Whether the second parameter is checked is
// controlled by the parameter `ParamsToCheckCount`.
static bool
-canOverloadedOperatorArgsBeModified(const FunctionDecl *OperatorDecl,
+canOverloadedOperatorArgsBeModified(const CXXOperatorCallExpr *OperatorCall,
bool checkSecondParam) {
+ const auto *OperatorDecl =
+ dyn_cast_or_null<FunctionDecl>(OperatorCall->getCalleeDecl());
+ // if we can't find the declaration, conservatively assume it can modify
+ // arguments
+ if (!OperatorDecl)
+ return true;
+
unsigned ParamCount = OperatorDecl->getNumParams();
// Overloaded operators declared inside a class have only one param.
@@ -527,14 +670,7 @@ static bool retrieveRelationalIntegerConstantExpr(
Value = APSInt(32, false);
} else if (const auto *OverloadedOperatorExpr =
Result.Nodes.getNodeAs<CXXOperatorCallExpr>(OverloadId)) {
- const auto *OverloadedFunctionDecl = dyn_cast_or_null<FunctionDecl>(OverloadedOperatorExpr->getCalleeDecl());
- if (!OverloadedFunctionDecl)
- return false;
-
- if (canOverloadedOperatorArgsBeModified(OverloadedFunctionDecl, false))
- return false;
-
- if (canOverloadedOperatorArgsBeModified(OverloadedFunctionDecl, false))
+ if (canOverloadedOperatorArgsBeModified(OverloadedOperatorExpr, false))
return false;
if (const auto *Arg = OverloadedOperatorExpr->getArg(1)) {
@@ -714,6 +850,21 @@ void RedundantExpressionCheck::registerMatchers(MatchFinder *Finder) {
.bind("binary"),
this);
+ // Logical or bitwise operator with equivalent nested operands, like (X && Y
+ // && X) or (X && (Y && X))
+ Finder->addMatcher(
+ binaryOperator(anyOf(hasOperatorName("|"), hasOperatorName("&"),
+ hasOperatorName("||"), hasOperatorName("&&"),
+ hasOperatorName("^")),
+ nestedOperandsAreEquivalent(),
+ // Filter noisy false positives.
+ unless(isInTemplateInstantiation()),
+ unless(binaryOperatorIsInMacro()),
+ // TODO: if the banned macros are themselves duplicated
+ unless(hasDescendant(BannedIntegerLiteral)))
+ .bind("nested-duplicates"),
+ this);
+
// Conditional (trenary) operator with equivalent operands, like (Y ? X : X).
Finder->addMatcher(conditionalOperator(expressionsAreEquivalent(),
// Filter noisy false positives.
@@ -740,6 +891,19 @@ void RedundantExpressionCheck::registerMatchers(MatchFinder *Finder) {
.bind("call"),
this);
+ // Overloaded operators with equivalent operands.
+ Finder->addMatcher(
+ cxxOperatorCallExpr(
+ anyOf(hasOverloadedOperatorName("|"), hasOverloadedOperatorName("&"),
+ hasOverloadedOperatorName("||"),
+ hasOverloadedOperatorName("&&"),
+ hasOverloadedOperatorName("^")),
+ nestedParametersAreEquivalent(), argumentCountIs(2),
+ // Filter noisy false positives.
+ unless(isMacro()), unless(isInTemplateInstantiation()))
+ .bind("nested-duplicates"),
+ this);
+
// Match expressions like: !(1 | 2 | 3)
Finder->addMatcher(
implicitCastExpr(
@@ -1061,17 +1225,29 @@ void RedundantExpressionCheck::check(const MatchFinder::MatchResult &Result) {
}
if (const auto *Call = Result.Nodes.getNodeAs<CXXOperatorCallExpr>("call")) {
- const auto *OverloadedFunctionDecl = dyn_cast_or_null<FunctionDecl>(Call->getCalleeDecl());
- if (!OverloadedFunctionDecl)
- return;
-
- if (canOverloadedOperatorArgsBeModified(OverloadedFunctionDecl, true))
+ if (canOverloadedOperatorArgsBeModified(Call, true))
return;
diag(Call->getOperatorLoc(),
"both sides of overloaded operator are equivalent");
}
+ if (const auto *Op = Result.Nodes.getNodeAs<Expr>("nested-duplicates")) {
+ const auto *Call = dyn_cast<CXXOperatorCallExpr>(Op);
+ if (Call && canOverloadedOperatorArgsBeModified(Call, true))
+ return;
+
+ StringRef Message =
+ Call ? "overloaded operator has equivalent nested operands"
+ : "operator has equivalent nested operands";
+
+ const auto Diag = diag(Op->getExprLoc(), Message);
+ for (const auto &KeyValue : Result.Nodes.getMap()) {
+ if (StringRef(KeyValue.first).startswith("duplicate"))
+ Diag << KeyValue.second.getSourceRange();
+ }
+ }
+
if (const auto *NegateOperator =
Result.Nodes.getNodeAs<UnaryOperator>("logical-bitwise-confusion")) {
SourceLocation OperatorLoc = NegateOperator->getOperatorLoc();
diff --git a/clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp b/clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp
index 500478159a42..4a4304be9bf4 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp
@@ -81,6 +81,13 @@ int TestSimpleEquivalent(int X, int Y) {
if (P2.a[P1.x + 2] < P2.x && P2.a[(P1.x) + (2)] < (P2.x)) return 1;
// CHECK-MESSAGES: :[[@LINE-1]]:29: warning: both sides of operator are equivalent
+ if (X && Y && X) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: operator has equivalent nested operands
+ if (X || (Y || X)) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: operator has equivalent nested operands
+ if ((X ^ Y) ^ (Y ^ X)) return 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: operator has equivalent nested operands
+
return 0;
}
@@ -106,6 +113,7 @@ int Valid(int X, int Y) {
if (++X != ++X) return 1;
if (P.a[X]++ != P.a[X]++) return 1;
if (P.a[X++] != P.a[X++]) return 1;
+ if (X && X++ && X) return 1;
if ("abc" == "ABC") return 1;
if (foo(bar(0)) < (foo(bat(0, 1)))) return 1;
@@ -163,6 +171,15 @@ bool operator<(const MyStruct& lhs, const MyStruct& rhs) { return lhs.x < rhs.x;
bool operator>(const MyStruct& lhs, MyStruct& rhs) { rhs.x--; return lhs.x > rhs.x; }
bool operator||(MyStruct& lhs, const MyStruct& rhs) { lhs.x++; return lhs.x || rhs.x; }
+struct MyStruct1 {
+ bool x;
+ MyStruct1(bool x) : x(x) {};
+ operator bool() { return x; }
+};
+
+MyStruct1 operator&&(const MyStruct1& lhs, const MyStruct1& rhs) { return lhs.x && rhs.x; }
+MyStruct1 operator||(MyStruct1& lhs, MyStruct1& rhs) { return lhs.x && rhs.x; }
+
bool TestOverloadedOperator(MyStruct& S) {
if (S == Q) return false;
@@ -180,6 +197,15 @@ bool TestOverloadedOperator(MyStruct& S) {
if (S >= S) return true;
// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of overloaded operator are equivalent
+ MyStruct1 U(false);
+ MyStruct1 V(true);
+
+ // valid because the operator is not const
+ if ((U || V) || U) return true;
+
+ if (U && V && U && V) return true;
+ // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: overloaded operator has equivalent nested operands
+
return true;
}
More information about the cfe-commits
mailing list