steakhal updated this revision to Diff 390032.
steakhal marked 3 inline comments as done.
steakhal added a comment.
Fixing nits: renaming variables, etc.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D114622/new/
https://reviews.llvm.org/D114622
Files:
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp
clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
clang/test/Analysis/identical-expressions.cpp
Index: clang/test/Analysis/identical-expressions.cpp
===================================================================
--- clang/test/Analysis/identical-expressions.cpp
+++ clang/test/Analysis/identical-expressions.cpp
@@ -1562,3 +1562,50 @@
;
}
}
+
+template <bool V> struct boolean_value {
+ static constexpr bool value = V;
+};
+template <typename T> struct my_trait : boolean_value<true> {};
+
+bool respect_nested_name_specifiers(bool sink) {
+ sink |= my_trait<char>::value || my_trait<int>::value; // no-warning
+
+ sink |= my_trait<char>::value || my_trait<char>::value;
+ // expected-warning@-1 {{identical expressions on both sides of logical operator}}
+
+ using my_char = char;
+ sink |= my_trait<char>::value || my_trait<my_char>::value;
+ // expected-warning@-1 {{identical expressions on both sides of logical operator}}
+
+ return sink;
+}
+
+static void static_function() {}
+namespace my {
+int fn(int = 1);
+}
+void respect_namespaces(bool coin) {
+ namespace other = my;
+ using namespace other;
+
+ coin ? my::fn(1) : my::fn(2); // no-warning
+ coin ? my::fn(1) : other::fn(2); // no-warning
+
+ // expected-warning@+3 {{identical expressions on both sides of ':' in conditional expression}}
+ // expected-warning@+3 {{identical expressions on both sides of ':' in conditional expression}}
+ // expected-warning@+3 {{identical expressions on both sides of ':' in conditional expression}}
+ coin ? my::fn(1) : my::fn(1);
+ coin ? my::fn(1) : other::fn(1);
+ coin ? my::fn(1) : fn(1);
+
+ coin ? my::fn(1) : fn(); // FIXME: We should have a warning for this: the default parameter is also 1.
+
+ my::fn(1) & my::fn(1); // no-warning
+ my::fn(1) & other::fn(1); // no-warning
+
+ // expected-warning@+2 {{identical expressions on both sides of ':' in conditional expression}}
+ // expected-warning@+2 {{identical expressions on both sides of ':' in conditional expression}}
+ coin ? ::static_function() : ::static_function();
+ coin ? ::static_function() : static_function();
+}
Index: clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
@@ -295,6 +295,32 @@
return true;
}
+static bool areEquivalentNameSpecifier(const NestedNameSpecifier *Left,
+ const NestedNameSpecifier *Right) {
+ const Type *LTy = Left->getAsType();
+ const Type *RTy = Right->getAsType();
+ if (LTy && RTy)
+ return LTy->getCanonicalTypeUnqualified() ==
+ RTy->getCanonicalTypeUnqualified();
+
+ // FIXME: We should probably check the other kinds of nested name specifiers.
+ return true;
+}
+
+static bool areEquivalentDeclRefs(const DeclRefExpr *Left,
+ const DeclRefExpr *Right) {
+ if (Left->getDecl()->getCanonicalDecl() !=
+ Right->getDecl()->getCanonicalDecl()) {
+ return false;
+ }
+
+ // The decls were already matched, so just return true at any later point.
+ if (!Left->hasQualifier() || !Right->hasQualifier())
+ return true;
+ return areEquivalentNameSpecifier(Left->getQualifier(),
+ Right->getQualifier());
+}
+
/// Determines whether two statement trees are identical regarding
/// operators and symbols.
///
@@ -458,11 +484,9 @@
const CharacterLiteral *CharLit2 = cast<CharacterLiteral>(Stmt2);
return CharLit1->getValue() == CharLit2->getValue();
}
- case Stmt::DeclRefExprClass: {
- const DeclRefExpr *DeclRef1 = cast<DeclRefExpr>(Stmt1);
- const DeclRefExpr *DeclRef2 = cast<DeclRefExpr>(Stmt2);
- return DeclRef1->getDecl() == DeclRef2->getDecl();
- }
+ case Stmt::DeclRefExprClass:
+ return areEquivalentDeclRefs(cast<DeclRefExpr>(Stmt1),
+ cast<DeclRefExpr>(Stmt2));
case Stmt::IntegerLiteralClass: {
const IntegerLiteral *IntLit1 = cast<IntegerLiteral>(Stmt1);
const IntegerLiteral *IntLit2 = cast<IntegerLiteral>(Stmt2);
Index: clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp
@@ -807,3 +807,52 @@
};
} // namespace no_crash
+
+template <bool V> struct boolean_value {
+ static constexpr bool value = V;
+};
+template <typename T> struct my_trait : boolean_value<true> {};
+
+bool respect_nested_name_specifiers(bool sink) {
+ sink |= my_trait<char>::value || my_trait<int>::value; // no-warning
+
+ sink |= my_trait<char>::value || my_trait<char>::value;
+ // CHECK-MESSAGES: :[[@LINE-1]]:33: warning: both sides of operator are equivalent
+
+ using my_char = char;
+ sink |= my_trait<char>::value || my_trait<my_char>::value;
+ // CHECK-MESSAGES: :[[@LINE-1]]:33: warning: both sides of operator are equivalent
+
+ return sink;
+}
+
+static void static_function() {}
+namespace my {
+int fn(int = 1);
+}
+void respect_namespaces(bool coin) {
+ namespace other = my;
+ using namespace other;
+
+ coin ? my::fn(1) : my::fn(2); // no-warning
+ coin ? my::fn(1) : other::fn(2); // no-warning
+
+ // CHECK-MESSAGES: :[[@LINE+3]]:20: warning: 'true' and 'false' expressions are equivalent
+ // CHECK-MESSAGES: :[[@LINE+3]]:20: warning: 'true' and 'false' expressions are equivalent
+ // CHECK-MESSAGES: :[[@LINE+3]]:20: warning: 'true' and 'false' expressions are equivalent
+ coin ? my::fn(1) : my::fn(1);
+ coin ? my::fn(1) : other::fn(1);
+ coin ? my::fn(1) : fn(1);
+
+ coin ? my::fn(1) : fn(); // FIXME: We should have a warning for this: the default parameter is also 1.
+
+ // CHECK-MESSAGES: :[[@LINE+2]]:13: warning: both sides of operator are equivalent [misc-redundant-expression]
+ // CHECK-MESSAGES: :[[@LINE+2]]:13: warning: both sides of operator are equivalent [misc-redundant-expression]
+ my::fn(1) & my::fn(1);
+ my::fn(1) & other::fn(1);
+
+ // CHECK-MESSAGES: :[[@LINE+2]]:30: warning: 'true' and 'false' expressions are equivalent
+ // CHECK-MESSAGES: :[[@LINE+2]]:30: warning: 'true' and 'false' expressions are equivalent
+ coin ? ::static_function() : ::static_function();
+ coin ? ::static_function() : static_function();
+}
Index: clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
+++ clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
@@ -51,10 +51,28 @@
static bool areEquivalentNameSpecifier(const NestedNameSpecifier *Left,
const NestedNameSpecifier *Right) {
- llvm::FoldingSetNodeID LeftID, RightID;
- Left->Profile(LeftID);
- Right->Profile(RightID);
- return LeftID == RightID;
+ const Type *LTy = Left->getAsType();
+ const Type *RTy = Right->getAsType();
+ if (LTy && RTy)
+ return LTy->getCanonicalTypeUnqualified() ==
+ RTy->getCanonicalTypeUnqualified();
+
+ // FIXME: We should probably check the other kinds of nested name specifiers.
+ return true;
+}
+
+static bool areEquivalentDeclRefs(const DeclRefExpr *Left,
+ const DeclRefExpr *Right) {
+ if (Left->getDecl()->getCanonicalDecl() !=
+ Right->getDecl()->getCanonicalDecl()) {
+ return false;
+ }
+
+ // The decls were already matched, so just return true at any later point.
+ if (!Left->hasQualifier() || !Right->hasQualifier())
+ return true;
+ return areEquivalentNameSpecifier(Left->getQualifier(),
+ Right->getQualifier());
}
static bool areEquivalentExpr(const Expr *Left, const Expr *Right) {
@@ -112,8 +130,8 @@
cast<DependentScopeDeclRefExpr>(Left)->getQualifier(),
cast<DependentScopeDeclRefExpr>(Right)->getQualifier());
case Stmt::DeclRefExprClass:
- return cast<DeclRefExpr>(Left)->getDecl() ==
- cast<DeclRefExpr>(Right)->getDecl();
+ return areEquivalentDeclRefs(cast<DeclRefExpr>(Left),
+ cast<DeclRefExpr>(Right));
case Stmt::MemberExprClass:
return cast<MemberExpr>(Left)->getMemberDecl() ==
cast<MemberExpr>(Right)->getMemberDecl();
Index: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
@@ -507,7 +507,6 @@
}
};
-// NOLINTNEXTLINE(misc-redundant-expression): Seems to be a bogus warning.
static_assert(std::is_trivially_copyable<Mix>::value &&
std::is_trivially_move_constructible<Mix>::value &&
std::is_trivially_move_assignable<Mix>::value,
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
