steakhal updated this revision to Diff 395150.
steakhal added a comment.
Sorry for the late update, but here we are:
Now, I properly handle all kinds of `NestedNameSpecifiers`.
Basically, I verify if the decls are matching, then I use the nested name
specifiers for finding mismatches. I look through namespace aliases and I use
the canonical types.
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,62 @@
;
}
}
+
+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}}
+
+ sink |= my_trait<char>::value || ::my_trait<my_char>::value;
+ // expected-warning@-1 {{identical expressions on both sides of logical operator}}
+
+ using my_trait_alias = my_trait<my_char>;
+ sink |= my_trait<char>::value || my_trait_alias::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;
+ namespace other2 = other;
+ using namespace other;
+
+ coin ? my::fn(1) : my::fn(2); // no-warning
+ coin ? my::fn(1) : other::fn(2); // no-warning
+
+ // expected-warning@+5 {{identical expressions on both sides of ':' in conditional expression}}
+ // expected-warning@+5 {{identical expressions on both sides of ':' in conditional expression}}
+ // expected-warning@+5 {{identical expressions on both sides of ':' in conditional expression}}
+ // expected-warning@+5 {{identical expressions on both sides of ':' in conditional expression}}
+ // expected-warning@+5 {{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) : other2::fn(1);
+ coin ? fn(1) : other2::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,81 @@
return true;
}
+static const NamespaceDecl *lookingThroughAliases(const NamedDecl *D) {
+ while (const auto *Alias = dyn_cast<NamespaceAliasDecl>(D))
+ D = Alias->getAliasedNamespace();
+ return cast_or_null<NamespaceDecl>(D);
+}
+
+static bool areEquivalentNameSpecifier(const NestedNameSpecifier *Left,
+ const NestedNameSpecifier *Right) {
+ const auto TryCompareAsNamespaces =
+ [](const NestedNameSpecifier *Left,
+ const NestedNameSpecifier *Right) -> Optional<bool> {
+ const NamespaceDecl *LeftAsNS = Left->getAsNamespace();
+ const NamespaceDecl *RightAsNS = Right->getAsNamespace();
+ if (const auto *LeftAlias = Left->getAsNamespaceAlias())
+ LeftAsNS = lookingThroughAliases(LeftAlias);
+ if (const auto *RightAlias = Left->getAsNamespaceAlias())
+ RightAsNS = lookingThroughAliases(RightAlias);
+
+ if (!LeftAsNS || !RightAsNS)
+ return None;
+ return LeftAsNS == RightAsNS;
+ };
+
+ const auto TryCompareAsTypes =
+ [](const NestedNameSpecifier *Left,
+ const NestedNameSpecifier *Right) -> Optional<bool> {
+ const Type *LeftAsTy = Left->getAsType();
+ const Type *RightAsTy = Right->getAsType();
+
+ if (!LeftAsTy || !RightAsTy)
+ return None;
+
+ LeftAsTy = LeftAsTy->getCanonicalTypeUnqualified().getTypePtr();
+ RightAsTy = RightAsTy->getCanonicalTypeUnqualified().getTypePtr();
+ return LeftAsTy == RightAsTy;
+ };
+
+ const auto TryCompareAsIdentifier =
+ [](const NestedNameSpecifier *Left,
+ const NestedNameSpecifier *Right) -> Optional<bool> {
+ const IdentifierInfo *LeftAsII = Left->getAsIdentifier();
+ const IdentifierInfo *RightAsII = Right->getAsIdentifier();
+ if (!LeftAsII || !RightAsII)
+ return None;
+ return LeftAsII == RightAsII;
+ };
+
+ while (Left && Right) {
+ if (!TryCompareAsNamespaces(Left, Right).getValueOr(true))
+ return false;
+ if (!TryCompareAsTypes(Left, Right).getValueOr(true))
+ return false;
+ if (!TryCompareAsIdentifier(Left, Right).getValueOr(true))
+ return false;
+
+ // Ignoring Global and Super NestedNameSpecifier kinds.
+ Left = Left->getPrefix();
+ Right = Right->getPrefix();
+ }
+
+ // We already know that the declrefs are refering to the same entity.
+ return true;
+}
+
+static bool areEquivalentDeclRefs(const DeclRefExpr *Left,
+ const DeclRefExpr *Right) {
+ if (Left->getDecl()->getCanonicalDecl() !=
+ Right->getDecl()->getCanonicalDecl()) {
+ return false;
+ }
+
+ return areEquivalentNameSpecifier(Left->getQualifier(),
+ Right->getQualifier());
+}
+
/// Determines whether two statement trees are identical regarding
/// operators and symbols.
///
@@ -458,11 +533,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,64 @@
};
} // 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
+
+ sink |= my_trait<char>::value || ::my_trait<my_char>::value;
+ // CHECK-MESSAGES: :[[@LINE-1]]:33: warning: both sides of operator are equivalent
+
+ using my_trait_alias = my_trait<my_char>;
+ sink |= my_trait<char>::value || my_trait_alias::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;
+ namespace other2 = other;
+ using namespace other;
+
+ coin ? my::fn(1) : my::fn(2); // no-warning
+ coin ? my::fn(1) : other::fn(2); // no-warning
+
+ // CHECK-MESSAGES: :[[@LINE+5]]:20: warning: 'true' and 'false' expressions are equivalent
+ // CHECK-MESSAGES: :[[@LINE+5]]:20: warning: 'true' and 'false' expressions are equivalent
+ // CHECK-MESSAGES: :[[@LINE+5]]:20: warning: 'true' and 'false' expressions are equivalent
+ // CHECK-MESSAGES: :[[@LINE+5]]:20: warning: 'true' and 'false' expressions are equivalent
+ // CHECK-MESSAGES: :[[@LINE+5]]:16: 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) : other2::fn(1);
+ coin ? fn(1) : other2::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
@@ -49,12 +49,87 @@
return Value < Result;
}
+static const NamespaceDecl *lookingThroughAliases(const NamedDecl *D) {
+ while (const auto *Alias = dyn_cast<NamespaceAliasDecl>(D))
+ D = Alias->getAliasedNamespace();
+ return cast_or_null<NamespaceDecl>(D);
+}
+
static bool areEquivalentNameSpecifier(const NestedNameSpecifier *Left,
const NestedNameSpecifier *Right) {
- llvm::FoldingSetNodeID LeftID, RightID;
- Left->Profile(LeftID);
- Right->Profile(RightID);
- return LeftID == RightID;
+ const auto TryCompareAsNamespaces =
+ [](const NestedNameSpecifier *Left,
+ const NestedNameSpecifier *Right) -> Optional<bool> {
+ const NamespaceDecl *LeftAsNS = Left->getAsNamespace();
+ const NamespaceDecl *RightAsNS = Right->getAsNamespace();
+ if (const auto *LeftAlias = Left->getAsNamespaceAlias())
+ LeftAsNS = lookingThroughAliases(LeftAlias);
+ if (const auto *RightAlias = Left->getAsNamespaceAlias())
+ RightAsNS = lookingThroughAliases(RightAlias);
+
+ if (!LeftAsNS || !RightAsNS)
+ return None;
+ return LeftAsNS == RightAsNS;
+ };
+
+ const auto TryCompareAsTypes =
+ [](const NestedNameSpecifier *Left,
+ const NestedNameSpecifier *Right) -> Optional<bool> {
+ const Type *LeftAsTy = Left->getAsType();
+ const Type *RightAsTy = Right->getAsType();
+
+ if (!LeftAsTy || !RightAsTy)
+ return None;
+
+ LeftAsTy = LeftAsTy->getCanonicalTypeUnqualified().getTypePtr();
+ RightAsTy = RightAsTy->getCanonicalTypeUnqualified().getTypePtr();
+ return LeftAsTy == RightAsTy;
+ };
+
+ const auto TryCompareAsIdentifier =
+ [](const NestedNameSpecifier *Left,
+ const NestedNameSpecifier *Right) -> Optional<bool> {
+ const IdentifierInfo *LeftAsII = Left->getAsIdentifier();
+ const IdentifierInfo *RightAsII = Right->getAsIdentifier();
+ if (!LeftAsII || !RightAsII)
+ return None;
+ return LeftAsII == RightAsII;
+ };
+
+ while (Left && Right) {
+ if (!TryCompareAsNamespaces(Left, Right).getValueOr(true))
+ return false;
+ if (!TryCompareAsTypes(Left, Right).getValueOr(true))
+ return false;
+ if (!TryCompareAsIdentifier(Left, Right).getValueOr(true))
+ return false;
+
+ // Ignoring Global and Super NestedNameSpecifier kinds.
+ Left = Left->getPrefix();
+ Right = Right->getPrefix();
+ }
+
+ // We already know that the declrefs are refering to the same entity.
+ return true;
+}
+
+static bool areEquivalentDeclRefs(const DeclRefExpr *Left,
+ const DeclRefExpr *Right) {
+ if (Left->getDecl()->getCanonicalDecl() !=
+ Right->getDecl()->getCanonicalDecl()) {
+ return false;
+ }
+
+ return areEquivalentNameSpecifier(Left->getQualifier(),
+ Right->getQualifier());
+}
+
+static bool areEquivalentDeclRefs(const DependentScopeDeclRefExpr *Left,
+ const DependentScopeDeclRefExpr *Right) {
+ if (Left->getDeclName() != Right->getDeclName())
+ return false;
+ return areEquivalentNameSpecifier(Left->getQualifier(),
+ Right->getQualifier());
}
static bool areEquivalentExpr(const Expr *Left, const Expr *Right) {
@@ -105,15 +180,11 @@
return cast<CXXOperatorCallExpr>(Left)->getOperator() ==
cast<CXXOperatorCallExpr>(Right)->getOperator();
case Stmt::DependentScopeDeclRefExprClass:
- if (cast<DependentScopeDeclRefExpr>(Left)->getDeclName() !=
- cast<DependentScopeDeclRefExpr>(Right)->getDeclName())
- return false;
- return areEquivalentNameSpecifier(
- cast<DependentScopeDeclRefExpr>(Left)->getQualifier(),
- cast<DependentScopeDeclRefExpr>(Right)->getQualifier());
+ return areEquivalentDeclRefs(cast<DependentScopeDeclRefExpr>(Left),
+ cast<DependentScopeDeclRefExpr>(Right));
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
