[clang-tools-extra] r269275 - [clang-tidy] Improve misc-redundant-expression and decrease false-positive
Etienne Bergeron via cfe-commits
cfe-commits at lists.llvm.org
Wed May 11 21:32:47 PDT 2016
Author: etienneb
Date: Wed May 11 23:32:47 2016
New Revision: 269275
URL: http://llvm.org/viewvc/llvm-project?rev=269275&view=rev
Log:
[clang-tidy] Improve misc-redundant-expression and decrease false-positive
Summary:
This patch is adding support for conditional expression and overloaded operators.
To decrease false-positive, this patch is adding a list of banned macro names that
has multiple variant with same integer value.
Also fixed support for template instantiation and added an unittest.
Reviewers: alexfh
Subscribers: klimek, Sarcasm, cfe-commits
Differential Revision: http://reviews.llvm.org/D19703
Modified:
clang-tools-extra/trunk/clang-tidy/misc/RedundantExpressionCheck.cpp
clang-tools-extra/trunk/docs/clang-tidy/checks/misc-redundant-expression.rst
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=269275&r1=269274&r2=269275&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/misc/RedundantExpressionCheck.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/misc/RedundantExpressionCheck.cpp Wed May 11 23:32:47 2016
@@ -9,8 +9,10 @@
#include "RedundantExpressionCheck.h"
#include "../utils/Matchers.h"
+#include "../utils/OptionsUtils.h"
#include "clang/AST/ASTContext.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
using namespace clang::ast_matchers;
@@ -18,7 +20,18 @@ namespace clang {
namespace tidy {
namespace misc {
-static bool AreIdenticalExpr(const Expr *Left, const Expr *Right) {
+static const char KnownBannedMacroNames[] =
+ "EAGAIN;EWOULDBLOCK;SIGCLD;SIGCHLD;";
+
+static bool areEquivalentNameSpecifier(const NestedNameSpecifier *Left,
+ const NestedNameSpecifier *Right) {
+ llvm::FoldingSetNodeID LeftID, RightID;
+ Left->Profile(LeftID);
+ Right->Profile(RightID);
+ return LeftID == RightID;
+}
+
+static bool areEquivalentExpr(const Expr *Left, const Expr *Right) {
if (!Left || !Right)
return !Left && !Right;
@@ -33,8 +46,8 @@ static bool AreIdenticalExpr(const Expr
Expr::const_child_iterator LeftIter = Left->child_begin();
Expr::const_child_iterator RightIter = Right->child_begin();
while (LeftIter != Left->child_end() && RightIter != Right->child_end()) {
- if (!AreIdenticalExpr(dyn_cast<Expr>(*LeftIter),
- dyn_cast<Expr>(*RightIter)))
+ if (!areEquivalentExpr(dyn_cast<Expr>(*LeftIter),
+ dyn_cast<Expr>(*RightIter)))
return false;
++LeftIter;
++RightIter;
@@ -53,7 +66,8 @@ static bool AreIdenticalExpr(const Expr
case Stmt::IntegerLiteralClass: {
llvm::APInt LeftLit = cast<IntegerLiteral>(Left)->getValue();
llvm::APInt RightLit = cast<IntegerLiteral>(Right)->getValue();
- return LeftLit.getBitWidth() == RightLit.getBitWidth() && LeftLit == RightLit;
+ return LeftLit.getBitWidth() == RightLit.getBitWidth() &&
+ LeftLit == RightLit;
}
case Stmt::FloatingLiteralClass:
return cast<FloatingLiteral>(Left)->getValue().bitwiseIsEqual(
@@ -62,6 +76,13 @@ static bool AreIdenticalExpr(const Expr
return cast<StringLiteral>(Left)->getBytes() ==
cast<StringLiteral>(Right)->getBytes();
+ case Stmt::DependentScopeDeclRefExprClass:
+ if (cast<DependentScopeDeclRefExpr>(Left)->getDeclName() !=
+ cast<DependentScopeDeclRefExpr>(Right)->getDeclName())
+ return false;
+ return areEquivalentNameSpecifier(
+ cast<DependentScopeDeclRefExpr>(Left)->getQualifier(),
+ cast<DependentScopeDeclRefExpr>(Right)->getQualifier());
case Stmt::DeclRefExprClass:
return cast<DeclRefExpr>(Left)->getDecl() ==
cast<DeclRefExpr>(Right)->getDecl();
@@ -89,22 +110,52 @@ static bool AreIdenticalExpr(const Expr
}
}
-AST_MATCHER(BinaryOperator, OperandsAreEquivalent) {
- return AreIdenticalExpr(Node.getLHS(), Node.getRHS());
+AST_MATCHER(BinaryOperator, operandsAreEquivalent) {
+ return areEquivalentExpr(Node.getLHS(), Node.getRHS());
+}
+
+AST_MATCHER(ConditionalOperator, expressionsAreEquivalent) {
+ return areEquivalentExpr(Node.getTrueExpr(), Node.getFalseExpr());
+}
+
+AST_MATCHER(CallExpr, parametersAreEquivalent) {
+ return Node.getNumArgs() == 2 &&
+ areEquivalentExpr(Node.getArg(0), Node.getArg(1));
}
-AST_MATCHER(BinaryOperator, isInMacro) {
+AST_MATCHER(BinaryOperator, binaryOperatorIsInMacro) {
return Node.getOperatorLoc().isMacroID();
}
-AST_MATCHER(Expr, isInstantiationDependent) {
- return Node.isInstantiationDependent();
+AST_MATCHER(ConditionalOperator, conditionalOperatorIsInMacro) {
+ return Node.getQuestionLoc().isMacroID() || Node.getColonLoc().isMacroID();
+}
+
+AST_MATCHER(Expr, isMacro) { return Node.getExprLoc().isMacroID(); }
+
+AST_MATCHER_P(Expr, expandedByMacro, std::set<std::string>, Names) {
+ const SourceManager &SM = Finder->getASTContext().getSourceManager();
+ const LangOptions &LO = Finder->getASTContext().getLangOpts();
+ SourceLocation Loc = Node.getExprLoc();
+ while (Loc.isMacroID()) {
+ std::string MacroName = Lexer::getImmediateMacroName(Loc, SM, LO);
+ if (Names.find(MacroName) != Names.end())
+ return true;
+ Loc = SM.getImmediateMacroCallerLoc(Loc);
+ }
+ return false;
}
void RedundantExpressionCheck::registerMatchers(MatchFinder *Finder) {
const auto AnyLiteralExpr = ignoringParenImpCasts(
anyOf(cxxBoolLiteral(), characterLiteral(), integerLiteral()));
+ std::vector<std::string> MacroNames =
+ utils::options::parseStringList(KnownBannedMacroNames);
+ std::set<std::string> Names(MacroNames.begin(), MacroNames.end());
+
+ const auto BannedIntegerLiteral = integerLiteral(expandedByMacro(Names));
+
Finder->addMatcher(
binaryOperator(anyOf(hasOperatorName("-"), hasOperatorName("/"),
hasOperatorName("%"), hasOperatorName("|"),
@@ -112,20 +163,52 @@ void RedundantExpressionCheck::registerM
matchers::isComparisonOperator(),
hasOperatorName("&&"), hasOperatorName("||"),
hasOperatorName("=")),
- OperandsAreEquivalent(),
+ operandsAreEquivalent(),
// Filter noisy false positives.
- unless(isInstantiationDependent()),
- unless(isInMacro()),
+ unless(isInTemplateInstantiation()),
+ unless(binaryOperatorIsInMacro()),
unless(hasType(realFloatingPointType())),
unless(hasEitherOperand(hasType(realFloatingPointType()))),
- unless(hasLHS(AnyLiteralExpr)))
+ unless(hasLHS(AnyLiteralExpr)),
+ unless(hasDescendant(BannedIntegerLiteral)))
.bind("binary"),
this);
+
+ Finder->addMatcher(
+ conditionalOperator(expressionsAreEquivalent(),
+ // Filter noisy false positives.
+ unless(conditionalOperatorIsInMacro()),
+ unless(hasTrueExpression(AnyLiteralExpr)),
+ unless(isInTemplateInstantiation()))
+ .bind("cond"),
+ this);
+
+ Finder->addMatcher(
+ cxxOperatorCallExpr(
+ anyOf(
+ hasOverloadedOperatorName("-"), hasOverloadedOperatorName("/"),
+ hasOverloadedOperatorName("%"), hasOverloadedOperatorName("|"),
+ hasOverloadedOperatorName("&"), hasOverloadedOperatorName("^"),
+ hasOverloadedOperatorName("=="), hasOverloadedOperatorName("!="),
+ hasOverloadedOperatorName("<"), hasOverloadedOperatorName("<="),
+ hasOverloadedOperatorName(">"), hasOverloadedOperatorName(">="),
+ hasOverloadedOperatorName("&&"), hasOverloadedOperatorName("||"),
+ hasOverloadedOperatorName("=")),
+ parametersAreEquivalent(),
+ // Filter noisy false positives.
+ unless(isMacro()), unless(isInTemplateInstantiation()))
+ .bind("call"),
+ this);
}
void RedundantExpressionCheck::check(const MatchFinder::MatchResult &Result) {
if (const auto *BinOp = Result.Nodes.getNodeAs<BinaryOperator>("binary"))
diag(BinOp->getOperatorLoc(), "both side of operator are equivalent");
+ if (const auto *CondOp = Result.Nodes.getNodeAs<ConditionalOperator>("cond"))
+ diag(CondOp->getColonLoc(), "'true' and 'false' expression are equivalent");
+ if (const auto *Call = Result.Nodes.getNodeAs<CXXOperatorCallExpr>("call"))
+ diag(Call->getOperatorLoc(),
+ "both side of overloaded operator are equivalent");
}
} // namespace misc
Modified: clang-tools-extra/trunk/docs/clang-tidy/checks/misc-redundant-expression.rst
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/clang-tidy/checks/misc-redundant-expression.rst?rev=269275&r1=269274&r2=269275&view=diff
==============================================================================
--- clang-tools-extra/trunk/docs/clang-tidy/checks/misc-redundant-expression.rst (original)
+++ clang-tools-extra/trunk/docs/clang-tidy/checks/misc-redundant-expression.rst Wed May 11 23:32:47 2016
@@ -12,6 +12,7 @@ Depending on the operator expressions ma
* always be a constant (zero or one)
Example:
+
.. code:: c++
((x+1) | (x+1)) // (x+1) is redundant
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=269275&r1=269274&r2=269275&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 Wed May 11 23:32:47 2016
@@ -68,14 +68,14 @@ int Test(int X, int Y) {
// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: both side of operator are equivalent
if (foo(0) - 2 < foo(0) - 2) return 1;
- // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: both side of operator are equivalent
+ // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: both side of operator are equivalent
if (foo(bar(0)) < (foo(bar((0))))) return 1;
- // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: both side of operator are equivalent
+ // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: both side of operator are equivalent
if (P1.x < P2.x && P1.x < P2.x) return 1;
- // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: both side of operator are equivalent
+ // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: both side of operator are equivalent
if (P2.a[P1.x + 2] < P2.x && P2.a[(P1.x) + (2)] < (P2.x)) return 1;
- // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: both side of operator are equivalent
+ // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: both side of operator are equivalent
return 0;
}
@@ -100,13 +100,36 @@ int Valid(int X, int Y) {
return 0;
}
+int TestConditional(int x, int y) {
+ int k = 0;
+ k += (y < 0) ? x : x;
+ // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: 'true' and 'false' expression are equivalent
+ k += (y < 0) ? x + 1 : x + 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: 'true' and 'false' expression are equivalent
+ return k;
+}
+
+struct MyStruct {
+ int x;
+} Q;
+bool operator==(const MyStruct& lhs, const MyStruct& rhs) { return lhs.x == rhs.x; }
+bool TestOperator(const MyStruct& S) {
+ if (S == Q) return false;
+ return S == S;
+ // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: both side of overloaded operator are equivalent
+}
+
#define LT(x, y) (void)((x) < (y))
+#define COND(x, y, z) ((x)?(y):(z))
+#define EQUALS(x, y) (x) == (y)
int TestMacro(int X, int Y) {
LT(0, 0);
LT(1, 0);
LT(X, X);
LT(X+1, X + 1);
+ COND(X < Y, X, X);
+ EQUALS(Q, Q);
}
int TestFalsePositive(int* A, int X, float F) {
@@ -118,3 +141,22 @@ int TestFalsePositive(int* A, int X, flo
if (F != F && F == F) return 1;
return 0;
}
+
+int TestBannedMacros() {
+#define EAGAIN 3
+#define NOT_EAGAIN 3
+ if (EAGAIN == 0 | EAGAIN == 0) return 0;
+ if (NOT_EAGAIN == 0 | NOT_EAGAIN == 0) return 0;
+ // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: both side of operator are equivalent
+}
+
+struct MyClass {
+static const int Value = 42;
+};
+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 side of overloaded operator are equivalent
+}
+void TestTemplate() { TemplateCheck<MyClass, MyClass>(); }
More information about the cfe-commits
mailing list