[clang-tools-extra] r319033 - [clang-tidy] Misc redundant expressions check updated for overloaded operators
Gabor Horvath via cfe-commits
cfe-commits at lists.llvm.org
Mon Nov 27 07:05:24 PST 2017
Author: xazax
Date: Mon Nov 27 07:05:24 2017
New Revision: 319033
URL: http://llvm.org/viewvc/llvm-project?rev=319033&view=rev
Log:
[clang-tidy] Misc redundant expressions check updated for overloaded operators
Patch by: Lilla Barancsuk
Differential Revision: https://reviews.llvm.org/D39243
Modified:
clang-tools-extra/trunk/clang-tidy/misc/RedundantExpressionCheck.cpp
clang-tools-extra/trunk/test/clang-tidy/misc-redundant-expression.cpp
Modified: clang-tools-extra/trunk/clang-tidy/misc/RedundantExpressionCheck.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/misc/RedundantExpressionCheck.cpp?rev=319033&r1=319032&r2=319033&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/misc/RedundantExpressionCheck.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/misc/RedundantExpressionCheck.cpp Mon Nov 27 07:05:24 2017
@@ -98,6 +98,9 @@ static bool areEquivalentExpr(const Expr
case Stmt::StringLiteralClass:
return cast<StringLiteral>(Left)->getBytes() ==
cast<StringLiteral>(Right)->getBytes();
+ case Stmt::CXXOperatorCallExprClass:
+ return cast<CXXOperatorCallExpr>(Left)->getOperator() ==
+ cast<CXXOperatorCallExpr>(Right)->getOperator();
case Stmt::DependentScopeDeclRefExprClass:
if (cast<DependentScopeDeclRefExpr>(Left)->getDeclName() !=
cast<DependentScopeDeclRefExpr>(Right)->getDeclName())
@@ -410,6 +413,7 @@ matchRelationalIntegerConstantExpr(Strin
std::string CastId = (Id + "-cast").str();
std::string SwapId = (Id + "-swap").str();
std::string NegateId = (Id + "-negate").str();
+ std::string OverloadId = (Id + "-overload").str();
const auto RelationalExpr = ignoringParenImpCasts(binaryOperator(
isComparisonOperator(), expr().bind(Id),
@@ -437,12 +441,54 @@ matchRelationalIntegerConstantExpr(Strin
hasOperatorName("!"),
hasUnaryOperand(anyOf(CastExpr, RelationalExpr)))));
+ const auto OverloadedOperatorExpr =
+ cxxOperatorCallExpr(
+ anyOf(hasOverloadedOperatorName("=="),
+ hasOverloadedOperatorName("!="), hasOverloadedOperatorName("<"),
+ hasOverloadedOperatorName("<="), hasOverloadedOperatorName(">"),
+ hasOverloadedOperatorName(">=")),
+ // Filter noisy false positives.
+ unless(isMacro()), unless(isInTemplateInstantiation()))
+ .bind(OverloadId);
+
return anyOf(RelationalExpr, CastExpr, NegateRelationalExpr,
- NegateNegateRelationalExpr);
+ NegateNegateRelationalExpr, OverloadedOperatorExpr);
}
-// Retrieves sub-expressions matched by 'matchRelationalIntegerConstantExpr' with
-// name 'Id'.
+// Checks whether a function param is non constant reference type, and may
+// be modified in the function.
+static bool isNonConstReferenceType(QualType ParamType) {
+ return ParamType->isReferenceType() &&
+ !ParamType.getNonReferenceType().isConstQualified();
+}
+
+// Checks whether the arguments of an overloaded operator can be modified in the
+// function.
+// For operators that take an instance and a constant as arguments, only the
+// first argument (the instance) needs to be checked, since the constant itself
+// is a temporary expression. Whether the second parameter is checked is
+// controlled by the parameter `ParamsToCheckCount`.
+static bool
+canOverloadedOperatorArgsBeModified(const FunctionDecl *OperatorDecl,
+ bool checkSecondParam) {
+ unsigned ParamCount = OperatorDecl->getNumParams();
+
+ // Overloaded operators declared inside a class have only one param.
+ // These functions must be declared const in order to not be able to modify
+ // the instance of the class they are called through.
+ if (ParamCount == 1 &&
+ !OperatorDecl->getType()->getAs<FunctionType>()->isConst())
+ return true;
+
+ if (isNonConstReferenceType(OperatorDecl->getParamDecl(0)->getType()))
+ return true;
+
+ return checkSecondParam && ParamCount == 2 &&
+ isNonConstReferenceType(OperatorDecl->getParamDecl(1)->getType());
+}
+
+// Retrieves sub-expressions matched by 'matchRelationalIntegerConstantExpr'
+// with name 'Id'.
static bool retrieveRelationalIntegerConstantExpr(
const MatchFinder::MatchResult &Result, StringRef Id,
const Expr *&OperandExpr, BinaryOperatorKind &Opcode, const Expr *&Symbol,
@@ -450,6 +496,7 @@ static bool retrieveRelationalIntegerCon
std::string CastId = (Id + "-cast").str();
std::string SwapId = (Id + "-swap").str();
std::string NegateId = (Id + "-negate").str();
+ std::string OverloadId = (Id + "-overload").str();
if (const auto *Bin = Result.Nodes.getNodeAs<BinaryOperator>(Id)) {
// Operand received with explicit comparator.
@@ -458,12 +505,29 @@ static bool retrieveRelationalIntegerCon
if (!retrieveIntegerConstantExpr(Result, Id, Value, ConstExpr))
return false;
-
} else if (const auto *Cast = Result.Nodes.getNodeAs<CastExpr>(CastId)) {
// Operand received with implicit comparator (cast).
Opcode = BO_NE;
OperandExpr = Cast;
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 (!OverloadedOperatorExpr->getArg(1)->isIntegerConstantExpr(
+ Value, *Result.Context))
+ return false;
+
+ Symbol = OverloadedOperatorExpr->getArg(0);
+ OperandExpr = OverloadedOperatorExpr;
+ Opcode = BinaryOperator::getOverloadedOpcode(OverloadedOperatorExpr->getOperator());
+
+ return BinaryOperator::isComparisonOp(Opcode);
} else {
return false;
}
@@ -548,7 +612,8 @@ static bool areExprsFromDifferentMacros(
Lexer::getImmediateMacroName(RhsLoc, SM, LO));
}
-static bool areExprsMacroAndNonMacro(const Expr *&LhsExpr, const Expr *&RhsExpr) {
+static bool areExprsMacroAndNonMacro(const Expr *&LhsExpr,
+ const Expr *&RhsExpr) {
if (!LhsExpr || !RhsExpr)
return false;
@@ -562,7 +627,8 @@ void RedundantExpressionCheck::registerM
const auto AnyLiteralExpr = ignoringParenImpCasts(
anyOf(cxxBoolLiteral(), characterLiteral(), integerLiteral()));
- const auto BannedIntegerLiteral = integerLiteral(expandedByMacro(KnownBannedMacroNames));
+ const auto BannedIntegerLiteral =
+ integerLiteral(expandedByMacro(KnownBannedMacroNames));
// Binary with equivalent operands, like (X != 2 && X != 2).
Finder->addMatcher(
@@ -584,13 +650,12 @@ void RedundantExpressionCheck::registerM
this);
// Conditional (trenary) operator with equivalent operands, like (Y ? X : X).
- Finder->addMatcher(
- conditionalOperator(expressionsAreEquivalent(),
- // Filter noisy false positives.
- unless(conditionalOperatorIsInMacro()),
- unless(isInTemplateInstantiation()))
- .bind("cond"),
- this);
+ Finder->addMatcher(conditionalOperator(expressionsAreEquivalent(),
+ // Filter noisy false positives.
+ unless(conditionalOperatorIsInMacro()),
+ unless(isInTemplateInstantiation()))
+ .bind("cond"),
+ this);
// Overloaded operators with equivalent operands.
Finder->addMatcher(
@@ -821,16 +886,15 @@ void RedundantExpressionCheck::checkRela
void RedundantExpressionCheck::check(const MatchFinder::MatchResult &Result) {
if (const auto *BinOp = Result.Nodes.getNodeAs<BinaryOperator>("binary")) {
-
// If the expression's constants are macros, check whether they are
// intentional.
if (areSidesBinaryConstExpressions(BinOp, Result.Context)) {
const Expr *LhsConst = nullptr, *RhsConst = nullptr;
BinaryOperatorKind MainOpcode, SideOpcode;
- if(!retrieveConstExprFromBothSides(BinOp, MainOpcode, SideOpcode, LhsConst,
- RhsConst, Result.Context))
- return;
+ if (!retrieveConstExprFromBothSides(BinOp, MainOpcode, SideOpcode,
+ LhsConst, RhsConst, Result.Context))
+ return;
if (areExprsFromDifferentMacros(LhsConst, RhsConst, Result.Context) ||
areExprsMacroAndNonMacro(LhsConst, RhsConst))
@@ -853,6 +917,13 @@ void RedundantExpressionCheck::check(con
}
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))
+ return;
+
diag(Call->getOperatorLoc(),
"both sides of overloaded operator are equivalent");
}
Modified: clang-tools-extra/trunk/test/clang-tidy/misc-redundant-expression.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/misc-redundant-expression.cpp?rev=319033&r1=319032&r2=319033&view=diff
==============================================================================
--- clang-tools-extra/trunk/test/clang-tidy/misc-redundant-expression.cpp (original)
+++ clang-tools-extra/trunk/test/clang-tidy/misc-redundant-expression.cpp Mon Nov 27 07:05:24 2017
@@ -124,16 +124,36 @@ int TestConditional(int x, int y) {
#undef COND_OP_MACRO
#undef COND_OP_OTHER_MACRO
+// Overloaded operators that compare two instances of a struct.
struct MyStruct {
- int x;
+ int x;
+ bool operator==(const MyStruct& rhs) const {return this->x == rhs.x; } // not modifing
+ bool operator>=(const MyStruct& rhs) const { return this->x >= rhs.x; } // not modifing
+ bool operator<=(MyStruct& rhs) const { return this->x <= rhs.x; }
+ bool operator&&(const MyStruct& rhs){ this->x++; return this->x && rhs.x; }
} Q;
-bool operator==(const MyStruct& lhs, const MyStruct& rhs) { return lhs.x == rhs.x; }
+bool operator!=(const MyStruct& lhs, const MyStruct& rhs) { return lhs.x == rhs.x; } // not modifing
+bool operator<(const MyStruct& lhs, const MyStruct& rhs) { return lhs.x < rhs.x; } // not modifing
+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; }
-bool TestOperator(MyStruct& S) {
+bool TestOverloadedOperator(MyStruct& S) {
if (S == Q) return false;
+
+ if (S <= S) return false;
+ if (S && S) return false;
+ if (S > S) return false;
+ if (S || S) return false;
+
if (S == S) return true;
// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of overloaded operator are equivalent
+ if (S < S) return true;
+ // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of overloaded operator are equivalent
+ if (S != S) return true;
+ // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of overloaded operator are equivalent
+ if (S >= S) return true;
+ // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of overloaded operator are equivalent
}
#define LT(x, y) (void)((x) < (y))
@@ -176,7 +196,7 @@ template <typename T, typename U>
void TemplateCheck() {
static_assert(T::Value == U::Value, "should be identical");
static_assert(T::Value == T::Value, "should be identical");
- // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: both sides of overloaded operator are equivalent
+ // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: both sides of operator are equivalent
}
void TestTemplate() { TemplateCheck<MyClass, MyClass>(); }
@@ -281,9 +301,46 @@ int TestBitwise(int X, int Y) {
return 0;
}
+// Overloaded operators that compare an instance of a struct and an integer
+// constant.
+struct S {
+ S() { x = 1; }
+ int x;
+ // Overloaded comparison operators without any possible side effect.
+ bool operator==(const int &i) const { return x == i; } // not modifying
+ bool operator!=(int i) const { return x != i; } // not modifying
+ bool operator>(const int &i) const { return x > i; } // not modifying
+ bool operator<(int i) const { return x < i; } // not modifying
+};
+
+bool operator<=(const S &s, int i) { return s.x <= i; } // not modifying
+bool operator>=(const S &s, const int &i) { return s.x >= i; } // not modifying
+
+struct S2 {
+ S2() { x = 1; }
+ int x;
+ // Overloaded comparison operators that are able to modify their params.
+ bool operator==(const int &i) {
+ this->x++;
+ return x == i;
+ }
+ bool operator!=(int i) { return x != i; }
+ bool operator>(const int &i) { return x > i; }
+ bool operator<(int i) {
+ this->x--;
+ return x < i;
+ }
+};
+
+bool operator>=(S2 &s, const int &i) { return s.x >= i; }
+bool operator<=(S2 &s, int i) {
+ s.x++;
+ return s.x <= i;
+}
+
int TestLogical(int X, int Y){
#define CONFIG 0
- if (CONFIG && X) return 1; // OK, consts from macros are considered intentional
+ if (CONFIG && X) return 1;
#undef CONFIG
#define CONFIG 1
if (CONFIG || X) return 1;
@@ -331,6 +388,24 @@ int TestLogical(int X, int Y){
if (!X && Y) return 1;
if (!X && Y == 0) return 1;
if (X == 10 && Y != 10) return 1;
+
+ // Test for overloaded operators with constant params.
+ S s1;
+ if (s1 == 1 && s1 == 1) return true;
+ // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: equivalent expression on both sides of logical operator
+ if (s1 == 1 || s1 != 1) return true;
+ // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: logical expression is always true
+ if (s1 > 1 && s1 < 1) return true;
+ // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: logical expression is always false
+ if (s1 >= 1 || s1 <= 1) return true;
+ // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: logical expression is always true
+
+ // Test for overloaded operators that may modify their params.
+ S2 s2;
+ if (s2 == 1 || s2 != 1) return true;
+ if (s2 == 1 || s2 == 1) return true;
+ if (s2 > 1 && s2 < 1) return true;
+ if (s2 >= 1 || s2 <= 1) return true;
}
int TestRelational(int X, int Y) {
More information about the cfe-commits
mailing list