diff --git a/clang-tools-extra/clang-tidy/readability/CMakeLists.txt b/clang-tools-extra/clang-tidy/readability/CMakeLists.txt --- a/clang-tools-extra/clang-tidy/readability/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/readability/CMakeLists.txt @@ -7,6 +7,7 @@ AvoidConstParamsInDecls.cpp BracesAroundStatementsCheck.cpp ConstReturnTypeCheck.cpp + ContainerContainsCheck.cpp ContainerDataPointerCheck.cpp ContainerSizeEmptyCheck.cpp ConvertMemberFunctionsToStatic.cpp diff --git a/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.h b/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.h new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.h @@ -0,0 +1,40 @@ +//===--- ContainerContainsCheck.h - clang-tidy ------------------*- C++ -*-===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_CONTAINERCONTAINSCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_CONTAINERCONTAINSCHECK_H + +#include "../ClangTidyCheck.h" + +namespace clang { +namespace tidy { +namespace readability { + +/// Finds usages of `container.count()` which should be replaced by a call +/// to the `container.contains()` method introduced in C++ 20. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/readability-container-contains.html +class ContainerContainsCheck : public ClangTidyCheck { +public: + ContainerContainsCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) final; + void check(const ast_matchers::MatchFinder::MatchResult &Result) final; + +protected: + bool isLanguageVersionSupported(const LangOptions &LO) const final { + return LO.CPlusPlus20; + } +}; + +} // namespace readability +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_CONTAINERCONTAINSCHECK_H diff --git a/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp b/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp @@ -0,0 +1,131 @@ +//===--- ContainerContainsCheck.cpp - clang-tidy --------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "ContainerContainsCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace readability { + +void ContainerContainsCheck::registerMatchers(MatchFinder *Finder) { + const auto SupportedContainers = hasType(cxxRecordDecl(hasAnyName( + "::std::set", "::std::unordered_set", "::std::map", + "::std::unordered_map", "::std::multiset", "::std::unordered_multiset", + "::std::multimap", "::std::unordered_multimap"))); + + const auto CountCall = + cxxMemberCallExpr(on(SupportedContainers), + callee(cxxMethodDecl(hasName("count"))), + argumentCountIs(1)) + .bind("call"); + + const auto FindCall = + cxxMemberCallExpr(on(SupportedContainers), + callee(cxxMethodDecl(hasName("find"))), + argumentCountIs(1)) + .bind("call"); + + const auto EndCall = cxxMemberCallExpr(on(SupportedContainers), + callee(cxxMethodDecl(hasName("end"))), + argumentCountIs(0)); + + const auto Literal0 = integerLiteral(equals(0)); + const auto Literal1 = integerLiteral(equals(1)); + + auto addSimpleMatcher = [&](auto matcher) { + Finder->addMatcher( + traverse(TK_IgnoreUnlessSpelledInSource, std::move(matcher)), this); + }; + + // Find containment checks which use `count` + Finder->addMatcher(implicitCastExpr(hasImplicitDestinationType(booleanType()), + hasSourceExpression(CountCall)) + .bind("positive"), + this); + addSimpleMatcher( + binaryOperator(hasLHS(CountCall), hasOperatorName("!="), hasRHS(Literal0)) + .bind("positive")); + addSimpleMatcher( + binaryOperator(hasLHS(Literal0), hasOperatorName("!="), hasRHS(CountCall)) + .bind("positive")); + addSimpleMatcher( + binaryOperator(hasLHS(CountCall), hasOperatorName(">"), hasRHS(Literal0)) + .bind("positive")); + addSimpleMatcher( + binaryOperator(hasLHS(Literal0), hasOperatorName("<"), hasRHS(CountCall)) + .bind("positive")); + addSimpleMatcher( + binaryOperator(hasLHS(CountCall), hasOperatorName(">="), hasRHS(Literal1)) + .bind("positive")); + addSimpleMatcher( + binaryOperator(hasLHS(Literal1), hasOperatorName("<="), hasRHS(CountCall)) + .bind("positive")); + + // Find inverted containment checks which use `count` + addSimpleMatcher( + binaryOperator(hasLHS(CountCall), hasOperatorName("=="), hasRHS(Literal0)) + .bind("negative")); + addSimpleMatcher( + binaryOperator(hasLHS(Literal0), hasOperatorName("=="), hasRHS(CountCall)) + .bind("negative")); + addSimpleMatcher( + binaryOperator(hasLHS(CountCall), hasOperatorName("<="), hasRHS(Literal0)) + .bind("negative")); + addSimpleMatcher( + binaryOperator(hasLHS(Literal0), hasOperatorName(">="), hasRHS(CountCall)) + .bind("negative")); + addSimpleMatcher( + binaryOperator(hasLHS(CountCall), hasOperatorName("<"), hasRHS(Literal1)) + .bind("negative")); + addSimpleMatcher( + binaryOperator(hasLHS(Literal1), hasOperatorName(">"), hasRHS(CountCall)) + .bind("negative")); + + // Find containment checks based on `begin == end` + addSimpleMatcher( + binaryOperator(hasLHS(FindCall), hasOperatorName("!="), hasRHS(EndCall)) + .bind("positive")); + addSimpleMatcher( + binaryOperator(hasLHS(FindCall), hasOperatorName("=="), hasRHS(EndCall)) + .bind("negative")); +} + +void ContainerContainsCheck::check(const MatchFinder::MatchResult &Result) { + // Extract the ifnromation about the match + const auto *Call = Result.Nodes.getNodeAs("call"); + const auto *PositiveCheck = Result.Nodes.getNodeAs("positive"); + const auto *NegativeCheck = Result.Nodes.getNodeAs("negative"); + bool Negated = NegativeCheck != nullptr; + const auto *Check = Negated ? NegativeCheck : PositiveCheck; + + // Diagnose the issue + auto Diag = + diag(Call->getExprLoc(), + "use `contains` instead of `count` to check for containment"); + + // Create the fix it + const auto *Member = cast(Call->getCallee()); + Diag << FixItHint::CreateReplacement( + Member->getMemberNameInfo().getSourceRange(), "contains"); + SourceLocation CheckBegin = Check->getSourceRange().getBegin(); + SourceLocation CheckEnd = Check->getSourceRange().getEnd(); + SourceLocation CallBegin = Call->getSourceRange().getBegin(); + SourceLocation CallEnd = Call->getSourceRange().getEnd(); + Diag << FixItHint::CreateReplacement( + CharSourceRange::getCharRange(CheckBegin, CallBegin), Negated ? "!" : ""); + Diag << FixItHint::CreateRemoval(CharSourceRange::getCharRange( + CallEnd.getLocWithOffset(1), CheckEnd.getLocWithOffset(1))); +} + +} // namespace readability +} // namespace tidy +} // namespace clang diff --git a/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp b/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp --- a/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp @@ -12,6 +12,7 @@ #include "AvoidConstParamsInDecls.h" #include "BracesAroundStatementsCheck.h" #include "ConstReturnTypeCheck.h" +#include "ContainerContainsCheck.h" #include "ContainerDataPointerCheck.h" #include "ContainerSizeEmptyCheck.h" #include "ConvertMemberFunctionsToStatic.h" @@ -63,6 +64,8 @@ "readability-braces-around-statements"); CheckFactories.registerCheck( "readability-const-return-type"); + CheckFactories.registerCheck( + "readability-container-contains"); CheckFactories.registerCheck( "readability-container-data-pointer"); CheckFactories.registerCheck( diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -88,6 +88,12 @@ Finds virtual classes whose destructor is neither public and virtual nor protected and non-virtual. +- New :doc:`readability-container-contains + ` check. + + Finds usages of `container.count()` which should be replaced by a call + to the `container.contains()` method introduced in C++ 20. + - New :doc:`readability-identifier-length ` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -286,6 +286,7 @@ `readability-avoid-const-params-in-decls `_, `readability-braces-around-statements `_, "Yes" `readability-const-return-type `_, "Yes" + `readability-container-contains `_, "Yes" `readability-container-size-empty `_, "Yes" `readability-convert-member-functions-to-static `_, `readability-delete-null-pointer `_, "Yes" diff --git a/clang-tools-extra/docs/clang-tidy/checks/readability-container-contains.rst b/clang-tools-extra/docs/clang-tidy/checks/readability-container-contains.rst new file mode 100644 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/readability-container-contains.rst @@ -0,0 +1,25 @@ +.. title:: clang-tidy - readability-container-contains + +readability-container-contains +============================== + +Finds usages of `container.count()` which should be replaced by a call to the `container.contains()` method introduced in C++ 20. + +Whether an element is contained inside a container should be checked with `contains` instead of `count`. For containers which permit multiple entries per key (`multimap`, `multiset`, ...), `contains` is more efficient than `count` because `count` has to do unnecessary additional work. While this this performance difference does not exist for containes with only a single entry per key (`map`, `unordered_map`, ...), `contains` still conveys the intent better. + +Examples: + +=========================================== ============================== +Initial expression Result +------------------------------------------- ------------------------------ +``x.begin() == x.end()`` ``!myMap.contains(x)`` +``x.begin() != x.end()`` ``myMap.contains(x)`` +``if (myMap.count(x))`` ``if (myMap.contains(x))`` +``bool exists = myMap.count(x)`` ``bool exists = myMap.contains(x)`` +``bool exists = myMap.count(x) > 0`` ``bool exists = myMap.contains(x)`` +``bool exists = myMap.count(x) >= 1`` ``bool exists = myMap.contains(x)`` +``bool missing = myMap.count(x) == 0`` ``bool missing = !myMap.contains(x)`` +=========================================== ================ + +This check applies to `std::set`, `std::unordered_set`, `std::map`, `std::unordered_map` and the corresponding multi-key variants. +It is only active for C++20 and later, as the `contains` method was only added in C++20. diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp @@ -0,0 +1,111 @@ +// RUN: %check_clang_tidy -std=c++20 %s readability-container-contains %t + +// Some *very* simplified versions of `map` etc. +namespace std { + +template +struct map { + unsigned count(const Key &K) const; + bool contains(const Key &K) const; + void *find(const Key &K); + void *end(); +}; + +template +struct set { + unsigned count(const Key &K) const; + bool contains(const Key &K) const; +}; + +template +struct unordered_set { + unsigned count(const Key &K) const; + bool contains(const Key &K) const; +}; + +template +struct multimap { + unsigned count(const Key &K) const; + bool contains(const Key &K) const; +}; + +} // namespace std + +using namespace std; + +// Check that we detect various common ways to check for containment +int testDifferentCheckTypes(map &MyMap) { + if (MyMap.count(0)) + // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use `contains` instead of `count` to check for containment [readability-container-contains] + // CHECK-FIXES: if (MyMap.contains(0)) + return 1; + bool C1 = MyMap.count(1); + // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use `contains` instead of `count` to check for containment [readability-container-contains] + // CHECK-FIXES: bool C1 = MyMap.contains(1); + auto C2 = static_cast(MyMap.count(1)); + // CHECK-MESSAGES: :[[@LINE-1]]:37: warning: use `contains` instead of `count` to check for containment [readability-container-contains] + // CHECK-FIXES: auto C2 = static_cast(MyMap.contains(1)); + auto C3 = MyMap.count(2) != 0; + // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use `contains` instead of `count` to check for containment [readability-container-contains] + // CHECK-FIXES: auto C3 = MyMap.contains(2); + auto C4 = MyMap.count(3) > 0; + // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use `contains` instead of `count` to check for containment [readability-container-contains] + // CHECK-FIXES: auto C4 = MyMap.contains(3); + auto C5 = MyMap.count(4) >= 1; + // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use `contains` instead of `count` to check for containment [readability-container-contains] + // CHECK-FIXES: auto C5 = MyMap.contains(4); + auto C6 = MyMap.find(5) != MyMap.end(); + // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use `contains` instead of `count` to check for containment [readability-container-contains] + // CHECK-FIXES: auto C6 = MyMap.contains(5); + return C1 + C2 + C3 + C4 + C5 + C6; +} + +// Check that we detect various common ways to check for non-containment +int testNegativeChecks(map &MyMap) { + bool C1 = !MyMap.count(-1); + // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: use `contains` instead of `count` to check for containment [readability-container-contains] + // CHECK-FIXES: bool C1 = !MyMap.contains(-1); + auto C2 = MyMap.count(-2) == 0; + // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use `contains` instead of `count` to check for containment [readability-container-contains] + // CHECK-FIXES: auto C2 = !MyMap.contains(-2); + auto C3 = MyMap.count(-3) <= 0; + // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use `contains` instead of `count` to check for containment [readability-container-contains] + // CHECK-FIXES: auto C3 = !MyMap.contains(-3); + auto C4 = MyMap.count(-4) < 1; + // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use `contains` instead of `count` to check for containment [readability-container-contains] + // CHECK-FIXES: auto C4 = !MyMap.contains(-4); + auto C5 = MyMap.find(-5) == MyMap.end(); + // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use `contains` instead of `count` to check for containment [readability-container-contains] + // CHECK-FIXES: auto C5 = !MyMap.contains(-5); + return C1 + C2 + C3 + C4 + C5; +} + +// Check for various types +int testDifferentTypes(map &M, unordered_set &US, set &S, multimap &MM) { + bool C1 = M.count(1001); + // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: use `contains` instead of `count` to check for containment [readability-container-contains] + // CHECK-FIXES: bool C1 = M.contains(1001); + bool C2 = US.count(1002); + // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: use `contains` instead of `count` to check for containment [readability-container-contains] + // CHECK-FIXES: bool C2 = US.contains(1002); + bool C3 = S.count(1003); + // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: use `contains` instead of `count` to check for containment [readability-container-contains] + // CHECK-FIXES: bool C3 = S.contains(1003); + bool C4 = MM.count(1004); + // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: use `contains` instead of `count` to check for containment [readability-container-contains] + // CHECK-FIXES: bool C4 = MM.contains(1004); + return C1 + C2 + C3 + C4; +} + +// This is effectively a containment check, as the result is implicitely casted to `bool` +bool returnContains(map &M) { + return M.count(42); + // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use `contains` instead of `count` to check for containment [readability-container-contains] + // CHECK-FIXES: return M.contains(42); +} + +// This returns the actual count and should not be rewritten +int actualCount(multimap &M) { + return M.count(21); + // CHECK-FIXES: return M.count(21); +}