steakhal created this revision.
steakhal added reviewers: aaron.ballman, alexfh, hokein, courbet, NoQ,
xazax.hun, martong, whisperity, davrec.
Herald added subscribers: carlosgalvezp, manas, ASDenysPetrov, dkrupp,
donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, rnkovacs, szepet,
baloghadamsoftware, dylanmckay.
Herald added a reviewer: Szelethus.
steakhal requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.
Consider this example:
#include <type_traits>
using std::is_same;
bool top() {
return is_same<char, int>::value || is_same<char, long>::value;
}
We can see that the `value` static members are different, but in fact,
they refer to the same VarDecl entity. It is because both `is_same`
class instances inherit from the common `false_type` class, which
actually owns the `value` member.
The `alpha.core.IdenticalExpr` Static Analyzer checker actually warns
for this, since it only checked if the referred `Decls` are the same.
Interestingly, the clang-tidy `misc-redundant-expression` also reports
this is for the exact same reason, so I'm fixing both of them at the same
time.
This patch fixes this by checking if the `Decls` are the same and if they
are it will try to acquire the nested namespace specifier as a type and
compare them as well. If they are inequal, that means that the
//spelling// of the nested namespace specifiers actually differed, this
it's likely to be a false-positive.
I'd like to note that the checker/check was actually right about that
the expressions were semantically equal in that given context, however,
we still don't want these reports in general.
We could introduce checker options to finetune this behavior if needed.
PS: According to the code coverage of the test, all touched parts are
completely covered.
Thanks David Rector (@davrec) for the idea on the cfe-dev forum:
https://lists.llvm.org/pipermail/cfe-dev/2021-November/069389.html
https://reviews.llvm.org/D114622
Files:
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,31 @@
return true;
}
+static bool areEquivalentNameSpecifier(const NestedNameSpecifier *Left,
+ const NestedNameSpecifier *Right) {
+ if (const auto *L = Left->getAsType())
+ if (const auto *R = Right->getAsType())
+ return L->getCanonicalTypeUnqualified() ==
+ R->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.
///
@@ -461,7 +486,7 @@
case Stmt::DeclRefExprClass: {
const DeclRefExpr *DeclRef1 = cast<DeclRefExpr>(Stmt1);
const DeclRefExpr *DeclRef2 = cast<DeclRefExpr>(Stmt2);
- return DeclRef1->getDecl() == DeclRef2->getDecl();
+ return areEquivalentDeclRefs(DeclRef1, DeclRef2);
}
case Stmt::IntegerLiteralClass: {
const IntegerLiteral *IntLit1 = cast<IntegerLiteral>(Stmt1);
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,27 @@
static bool areEquivalentNameSpecifier(const NestedNameSpecifier *Left,
const NestedNameSpecifier *Right) {
- llvm::FoldingSetNodeID LeftID, RightID;
- Left->Profile(LeftID);
- Right->Profile(RightID);
- return LeftID == RightID;
+ if (const auto *L = Left->getAsType())
+ if (const auto *R = Right->getAsType())
+ return L->getCanonicalTypeUnqualified() ==
+ R->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) {
@@ -111,9 +128,11 @@
return areEquivalentNameSpecifier(
cast<DependentScopeDeclRefExpr>(Left)->getQualifier(),
cast<DependentScopeDeclRefExpr>(Right)->getQualifier());
- case Stmt::DeclRefExprClass:
- return cast<DeclRefExpr>(Left)->getDecl() ==
- cast<DeclRefExpr>(Right)->getDecl();
+ case Stmt::DeclRefExprClass: {
+ const DeclRefExpr *DeclRef1 = cast<DeclRefExpr>(Left);
+ const DeclRefExpr *DeclRef2 = cast<DeclRefExpr>(Right);
+ return areEquivalentDeclRefs(DeclRef1, DeclRef2);
+ }
case Stmt::MemberExprClass:
return cast<MemberExpr>(Left)->getMemberDecl() ==
cast<MemberExpr>(Right)->getMemberDecl();
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
