[clang-tidy] Misc redundant expressions check updated for overloaded operators
Patch by: Lilla Barancsuk
Differential Revision: https://reviews.llvm.org/D39243
llvm-svn: 319033
diff --git a/clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp b/clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
index 265cb38..b3bd516 100644
--- a/clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
+++ b/clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
@@ -98,6 +98,9 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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::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 @@
}
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");
}