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::value && std::is_trivially_move_constructible::value && std::is_trivially_move_assignable::value, 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(D)) + D = Alias->getAliasedNamespace(); + return cast_or_null(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 { + 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 { + 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 { + 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(Left)->getOperator() == cast(Right)->getOperator(); case Stmt::DependentScopeDeclRefExprClass: - if (cast(Left)->getDeclName() != - cast(Right)->getDeclName()) - return false; - return areEquivalentNameSpecifier( - cast(Left)->getQualifier(), - cast(Right)->getQualifier()); + return areEquivalentDeclRefs(cast(Left), + cast(Right)); case Stmt::DeclRefExprClass: - return cast(Left)->getDecl() == - cast(Right)->getDecl(); + return areEquivalentDeclRefs(cast(Left), + cast(Right)); 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,64 @@ }; } // 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 + + sink |= my_trait::value || ::my_trait::value; + // CHECK-MESSAGES: :[[@LINE-1]]:33: warning: both sides of operator are equivalent + + using my_trait_alias = my_trait; + sink |= my_trait::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/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(D)) + D = Alias->getAliasedNamespace(); + return cast_or_null(D); +} + +static bool areEquivalentNameSpecifier(const NestedNameSpecifier *Left, + const NestedNameSpecifier *Right) { + const auto TryCompareAsNamespaces = + [](const NestedNameSpecifier *Left, + const NestedNameSpecifier *Right) -> Optional { + 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 { + 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 { + 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(Stmt2); return CharLit1->getValue() == CharLit2->getValue(); } - case Stmt::DeclRefExprClass: { - const DeclRefExpr *DeclRef1 = cast(Stmt1); - const DeclRefExpr *DeclRef2 = cast(Stmt2); - return DeclRef1->getDecl() == DeclRef2->getDecl(); - } + case Stmt::DeclRefExprClass: + return areEquivalentDeclRefs(cast(Stmt1), + cast(Stmt2)); case Stmt::IntegerLiteralClass: { const IntegerLiteral *IntLit1 = cast(Stmt1); const IntegerLiteral *IntLit2 = cast(Stmt2); Index: clang/test/Analysis/identical-expressions.cpp =================================================================== --- clang/test/Analysis/identical-expressions.cpp +++ clang/test/Analysis/identical-expressions.cpp @@ -1562,3 +1562,62 @@ ; } } + +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}} + + sink |= my_trait::value || ::my_trait::value; + // expected-warning@-1 {{identical expressions on both sides of logical operator}} + + using my_trait_alias = my_trait; + sink |= my_trait::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(); +}