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(Left)->getQualifier(), cast(Right)->getQualifier()); - case Stmt::DeclRefExprClass: - return cast(Left)->getDecl() == - cast(Right)->getDecl(); + case Stmt::DeclRefExprClass: { + const DeclRefExpr *DeclRef1 = cast(Left); + const DeclRefExpr *DeclRef2 = cast(Right); + return areEquivalentDeclRefs(DeclRef1, DeclRef2); + } case Stmt::MemberExprClass: return cast(Left)->getMemberDecl() == cast(Right)->getMemberDecl(); 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 struct boolean_value { + static constexpr bool value = V; +}; +template struct my_trait : boolean_value {}; + +bool respect_nested_name_specifiers(bool sink) { + sink |= my_trait::value || my_trait::value; // no-warning + + sink |= my_trait::value || my_trait::value; + // CHECK-MESSAGES: :[[@LINE-1]]:33: warning: both sides of operator are equivalent + + using my_char = char; + sink |= my_trait::value || my_trait::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/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(Stmt1); const DeclRefExpr *DeclRef2 = cast(Stmt2); - return DeclRef1->getDecl() == DeclRef2->getDecl(); + return areEquivalentDeclRefs(DeclRef1, DeclRef2); } case Stmt::IntegerLiteralClass: { const IntegerLiteral *IntLit1 = cast(Stmt1); Index: clang/test/Analysis/identical-expressions.cpp =================================================================== --- clang/test/Analysis/identical-expressions.cpp +++ clang/test/Analysis/identical-expressions.cpp @@ -1562,3 +1562,50 @@ ; } } + +template struct boolean_value { + static constexpr bool value = V; +}; +template struct my_trait : boolean_value {}; + +bool respect_nested_name_specifiers(bool sink) { + sink |= my_trait::value || my_trait::value; // no-warning + + sink |= my_trait::value || my_trait::value; + // expected-warning@-1 {{identical expressions on both sides of logical operator}} + + using my_char = char; + sink |= my_trait::value || my_trait::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(); +}