avogelsgesang updated this revision to Diff 382982.
avogelsgesang added a comment.
Fix formatting; remove unrelated changes
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D112646/new/
https://reviews.llvm.org/D112646
Files:
clang-tools-extra/clang-tidy/readability/CMakeLists.txt
clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp
clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.h
clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/list.rst
clang-tools-extra/docs/clang-tidy/checks/readability-container-contains.rst
clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp
===================================================================
--- /dev/null
+++ 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 <class Key, class T>
+struct map {
+ unsigned count(const Key &K) const;
+ bool contains(const Key &K) const;
+ void *find(const Key &K);
+ void *end();
+};
+
+template <class Key>
+struct set {
+ unsigned count(const Key &K) const;
+ bool contains(const Key &K) const;
+};
+
+template <class Key>
+struct unordered_set {
+ unsigned count(const Key &K) const;
+ bool contains(const Key &K) const;
+};
+
+template <class Key, class T>
+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<int, int> &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<bool>(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<bool>(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<int, int> &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<int, int> &M, unordered_set<int> &US, set<int> &S, multimap<int, int> &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<int, int> &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<int, int> &M) {
+ return M.count(21);
+ // CHECK-FIXES: return M.count(21);
+}
Index: clang-tools-extra/docs/clang-tidy/checks/readability-container-contains.rst
===================================================================
--- /dev/null
+++ 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.
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===================================================================
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -286,6 +286,7 @@
`readability-avoid-const-params-in-decls <readability-avoid-const-params-in-decls.html>`_,
`readability-braces-around-statements <readability-braces-around-statements.html>`_, "Yes"
`readability-const-return-type <readability-const-return-type.html>`_, "Yes"
+ `readability-container-contains <readability-container-contains.html>`_, "Yes"
`readability-container-size-empty <readability-container-size-empty.html>`_, "Yes"
`readability-convert-member-functions-to-static <readability-convert-member-functions-to-static.html>`_,
`readability-delete-null-pointer <readability-delete-null-pointer.html>`_, "Yes"
Index: clang-tools-extra/docs/ReleaseNotes.rst
===================================================================
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ 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
+ <clang-tidy/checks/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
<clang-tidy/checks/readability-identifier-length>` check.
Index: clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
===================================================================
--- clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
+++ 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<ConstReturnTypeCheck>(
"readability-const-return-type");
+ CheckFactories.registerCheck<ContainerContainsCheck>(
+ "readability-container-contains");
CheckFactories.registerCheck<ContainerDataPointerCheck>(
"readability-container-data-pointer");
CheckFactories.registerCheck<ContainerSizeEmptyCheck>(
Index: clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.h
===================================================================
--- /dev/null
+++ 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
Index: clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp
===================================================================
--- /dev/null
+++ 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<CXXMemberCallExpr>("call");
+ const auto *PositiveCheck = Result.Nodes.getNodeAs<Expr>("positive");
+ const auto *NegativeCheck = Result.Nodes.getNodeAs<Expr>("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<MemberExpr>(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
Index: clang-tools-extra/clang-tidy/readability/CMakeLists.txt
===================================================================
--- clang-tools-extra/clang-tidy/readability/CMakeLists.txt
+++ 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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
