[clang-tools-extra] 3696c70 - [clang-tidy] Add `readability-container-contains` check

via cfe-commits cfe-commits at lists.llvm.org
Mon Jan 24 03:57:33 PST 2022


Author: Adrian Vogelsgesang
Date: 2022-01-24T12:57:18+01:00
New Revision: 3696c70e67d9b9e54307ef25077bae7a6f76636e

URL: https://github.com/llvm/llvm-project/commit/3696c70e67d9b9e54307ef25077bae7a6f76636e
DIFF: https://github.com/llvm/llvm-project/commit/3696c70e67d9b9e54307ef25077bae7a6f76636e.diff

LOG: [clang-tidy] Add `readability-container-contains` check

This commit introduces a new check `readability-container-contains` which finds
usages of `container.count()` and `container.find() != container.end()` and
instead recommends the `container.contains()` method introduced in C++20.

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 containers with only
a single entry per key (`map`, `unordered_map`, ...), `contains` still conveys
the intent better.

Reviewed By: xazax.hun, whisperity

Differential Revision: http://reviews.llvm.org/D112646

Added: 
    clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp
    clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.h
    clang-tools-extra/docs/clang-tidy/checks/readability-container-contains.rst
    clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp

Modified: 
    clang-tools-extra/clang-tidy/readability/CMakeLists.txt
    clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
    clang-tools-extra/docs/ReleaseNotes.rst
    clang-tools-extra/docs/clang-tidy/checks/list.rst

Removed: 
    


################################################################################
diff  --git a/clang-tools-extra/clang-tidy/readability/CMakeLists.txt b/clang-tools-extra/clang-tidy/readability/CMakeLists.txt
index 22ce8f62751ec..ea09b2193eb7c 100644
--- a/clang-tools-extra/clang-tidy/readability/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/readability/CMakeLists.txt
@@ -7,6 +7,7 @@ add_clang_library(clangTidyReadabilityModule
   AvoidConstParamsInDecls.cpp
   BracesAroundStatementsCheck.cpp
   ConstReturnTypeCheck.cpp
+  ContainerContainsCheck.cpp
   ContainerDataPointerCheck.cpp
   ContainerSizeEmptyCheck.cpp
   ConvertMemberFunctionsToStatic.cpp

diff  --git a/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp b/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp
new file mode 100644
index 0000000000000..7a20480fb501c
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp
@@ -0,0 +1,144 @@
+//===--- 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(
+      hasUnqualifiedDesugaredType(recordType(hasDeclaration(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 membership tests which use `count()`.
+  Finder->addMatcher(implicitCastExpr(hasImplicitDestinationType(booleanType()),
+                                      hasSourceExpression(CountCall))
+                         .bind("positiveComparison"),
+                     this);
+  AddSimpleMatcher(
+      binaryOperator(hasLHS(CountCall), hasOperatorName("!="), hasRHS(Literal0))
+          .bind("positiveComparison"));
+  AddSimpleMatcher(
+      binaryOperator(hasLHS(Literal0), hasOperatorName("!="), hasRHS(CountCall))
+          .bind("positiveComparison"));
+  AddSimpleMatcher(
+      binaryOperator(hasLHS(CountCall), hasOperatorName(">"), hasRHS(Literal0))
+          .bind("positiveComparison"));
+  AddSimpleMatcher(
+      binaryOperator(hasLHS(Literal0), hasOperatorName("<"), hasRHS(CountCall))
+          .bind("positiveComparison"));
+  AddSimpleMatcher(
+      binaryOperator(hasLHS(CountCall), hasOperatorName(">="), hasRHS(Literal1))
+          .bind("positiveComparison"));
+  AddSimpleMatcher(
+      binaryOperator(hasLHS(Literal1), hasOperatorName("<="), hasRHS(CountCall))
+          .bind("positiveComparison"));
+
+  // Find inverted membership tests which use `count()`.
+  AddSimpleMatcher(
+      binaryOperator(hasLHS(CountCall), hasOperatorName("=="), hasRHS(Literal0))
+          .bind("negativeComparison"));
+  AddSimpleMatcher(
+      binaryOperator(hasLHS(Literal0), hasOperatorName("=="), hasRHS(CountCall))
+          .bind("negativeComparison"));
+  AddSimpleMatcher(
+      binaryOperator(hasLHS(CountCall), hasOperatorName("<="), hasRHS(Literal0))
+          .bind("negativeComparison"));
+  AddSimpleMatcher(
+      binaryOperator(hasLHS(Literal0), hasOperatorName(">="), hasRHS(CountCall))
+          .bind("negativeComparison"));
+  AddSimpleMatcher(
+      binaryOperator(hasLHS(CountCall), hasOperatorName("<"), hasRHS(Literal1))
+          .bind("negativeComparison"));
+  AddSimpleMatcher(
+      binaryOperator(hasLHS(Literal1), hasOperatorName(">"), hasRHS(CountCall))
+          .bind("negativeComparison"));
+
+  // Find membership tests based on `find() == end()`.
+  AddSimpleMatcher(
+      binaryOperator(hasLHS(FindCall), hasOperatorName("!="), hasRHS(EndCall))
+          .bind("positiveComparison"));
+  AddSimpleMatcher(
+      binaryOperator(hasLHS(FindCall), hasOperatorName("=="), hasRHS(EndCall))
+          .bind("negativeComparison"));
+}
+
+void ContainerContainsCheck::check(const MatchFinder::MatchResult &Result) {
+  // Extract the information about the match
+  const auto *Call = Result.Nodes.getNodeAs<CXXMemberCallExpr>("call");
+  const auto *PositiveComparison =
+      Result.Nodes.getNodeAs<Expr>("positiveComparison");
+  const auto *NegativeComparison =
+      Result.Nodes.getNodeAs<Expr>("negativeComparison");
+  assert(
+      !PositiveComparison ||
+      !NegativeComparison &&
+          "only one of PositiveComparison or NegativeComparison should be set");
+  bool Negated = NegativeComparison != nullptr;
+  const auto *Comparison = Negated ? NegativeComparison : PositiveComparison;
+
+  // Diagnose the issue.
+  auto Diag =
+      diag(Call->getExprLoc(), "use 'contains' to check for membership");
+
+  // Don't fix it if it's in a macro invocation. Leave fixing it to the user.
+  SourceLocation FuncCallLoc = Comparison->getEndLoc();
+  if (!FuncCallLoc.isValid() || FuncCallLoc.isMacroID())
+    return;
+
+  // Create the fix it.
+  const auto *Member = cast<MemberExpr>(Call->getCallee());
+  Diag << FixItHint::CreateReplacement(
+      Member->getMemberNameInfo().getSourceRange(), "contains");
+  SourceLocation ComparisonBegin = Comparison->getSourceRange().getBegin();
+  SourceLocation ComparisonEnd = Comparison->getSourceRange().getEnd();
+  SourceLocation CallBegin = Call->getSourceRange().getBegin();
+  SourceLocation CallEnd = Call->getSourceRange().getEnd();
+  Diag << FixItHint::CreateReplacement(
+      CharSourceRange::getCharRange(ComparisonBegin, CallBegin),
+      Negated ? "!" : "");
+  Diag << FixItHint::CreateRemoval(CharSourceRange::getCharRange(
+      CallEnd.getLocWithOffset(1), ComparisonEnd.getLocWithOffset(1)));
+}
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang

diff  --git a/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.h b/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.h
new file mode 100644
index 0000000000000..0c2705d437797
--- /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()` and `find() == end()` 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/ReadabilityTidyModule.cpp b/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
index b0493d43ff318..6bbef6b7fa07c 100644
--- 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"
@@ -64,6 +65,8 @@ class ReadabilityModule : public ClangTidyModule {
         "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>(

diff  --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 060af42521552..ba0e530b7fec4 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -118,6 +118,12 @@ New checks
 
   Reports identifier with unicode right-to-left characters.
 
+- New :doc:`readability-container-contains
+  <clang-tidy/checks/readability-container-contains>` check.
+
+  Finds usages of ``container.count()`` and ``container.find() == container.end()`` which should
+  be replaced by a call to the ``container.contains()`` method introduced in C++20.
+
 - New :doc:`readability-container-data-pointer
   <clang-tidy/checks/readability-container-data-pointer>` check.
 

diff  --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst
index 5878345bdfcfd..fcf661a406959 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -290,6 +290,7 @@ Clang-Tidy Checks
    `readability-avoid-const-params-in-decls <readability-avoid-const-params-in-decls.html>`_, "Yes"
    `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-data-pointer <readability-container-data-pointer.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>`_, "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
index 0000000000000..07d1e352d3b1b
--- /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()`` and ``container.find() == container.end()`` 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``/``find`` because ``contains`` conveys the intent more clearly. Furthermore, for containers which permit multiple entries per key (``multimap``, ``multiset``, ...), ``contains`` is more efficient than ``count`` because ``count`` has to do unnecessary additional work.
+
+Examples:
+
+===========================================  ==============================
+Initial expression                           Result
+-------------------------------------------  ------------------------------
+``myMap.find(x) == myMap.end()``             ``!myMap.contains(x)``
+``myMap.find(x) != myMap.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
index 0000000000000..c4ea1e27e63e6
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp
@@ -0,0 +1,230 @@
+// 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
+
+// Check that we detect various common ways to check for membership
+int testDifferentCheckTypes(std::map<int, int> &MyMap) {
+  if (MyMap.count(0))
+    // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use 'contains' to check for membership [readability-container-contains]
+    // CHECK-FIXES: if (MyMap.contains(0))
+    return 1;
+  bool C1 = MyMap.count(1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [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' to check for membership [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' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C3 = MyMap.contains(2);
+  auto C4 = MyMap.count(3) > 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C4 = MyMap.contains(3);
+  auto C5 = MyMap.count(4) >= 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [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' to check for membership [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-membership
+int testNegativeChecks(std::map<int, int> &MyMap) {
+  bool C1 = !MyMap.count(-1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: bool C1 = !MyMap.contains(-1);
+  auto C2 = MyMap.count(-2) == 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C2 = !MyMap.contains(-2);
+  auto C3 = MyMap.count(-3) <= 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C3 = !MyMap.contains(-3);
+  auto C4 = MyMap.count(-4) < 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [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' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C5 = !MyMap.contains(-5);
+  return C1 + C2 + C3 + C4 + C5;
+}
+
+// Check for various types
+int testDifferentTypes(std::map<int, int> &M, std::unordered_set<int> &US, std::set<int> &S, std::multimap<int, int> &MM) {
+  bool C1 = M.count(1001);
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: bool C1 = M.contains(1001);
+  bool C2 = US.count(1002);
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: bool C2 = US.contains(1002);
+  bool C3 = S.count(1003);
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: bool C3 = S.contains(1003);
+  bool C4 = MM.count(1004);
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: bool C4 = MM.contains(1004);
+  return C1 + C2 + C3 + C4;
+}
+
+// The check detects all kind of `const`, reference, rvalue-reference and value types.
+int testQualifiedTypes(std::map<int, int> ValueM, std::map<int, int> &RefM, const std::map<int, int> &ConstRefM, std::map<int, int> &&RValueM) {
+  bool C1 = ValueM.count(2001);
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: bool C1 = ValueM.contains(2001);
+  bool C2 = RefM.count(2002);
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: bool C2 = RefM.contains(2002);
+  bool C3 = ConstRefM.count(2003);
+  // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: bool C3 = ConstRefM.contains(2003);
+  bool C4 = RValueM.count(2004);
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: bool C4 = RValueM.contains(2004);
+  return C1 + C2 + C3 + C4;
+}
+
+// This is effectively a membership check, as the result is implicitly casted
+// to `bool`.
+bool returnContains(std::map<int, int> &M) {
+  return M.count(42);
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: return M.contains(42);
+}
+
+// This returns the actual count and should not be rewritten
+int actualCount(std::multimap<int, int> &M) {
+  return M.count(21);
+  // NO-WARNING.
+  // CHECK-FIXES: return M.count(21);
+}
+
+// Check that we are not confused by aliases
+namespace s2 = std;
+using MyMapT = s2::map<int, int>;
+int typeAliases(MyMapT &MyMap) {
+  bool C1 = MyMap.count(99);
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: bool C1 = MyMap.contains(99);
+  return C1;
+}
+
+// Check that the tests also trigger for a local variable and not only for
+// function arguments.
+bool localVar() {
+  using namespace std;
+  map<int, int> LocalM;
+  return LocalM.count(42);
+  // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: return LocalM.contains(42);
+}
+
+// Check various usages of an actual `count` which isn't rewritten
+int nonRewrittenCount(std::multimap<int, int> &MyMap) {
+  // This is an actual test if we have at least 2 usages. Shouldn't be rewritten.
+  bool C1 = MyMap.count(1) >= 2;
+  // NO-WARNING.
+  // CHECK-FIXES: bool C1 = MyMap.count(1) >= 2;
+
+  // "< 0" makes little sense and is always `false`. Still, let's ensure we
+  // don't accidentally rewrite it to 'contains'.
+  bool C2 = MyMap.count(2) < 0;
+  // NO-WARNING.
+  // CHECK-FIXES: bool C2 = MyMap.count(2) < 0;
+
+  // The `count` is used in some more complicated formula.
+  bool C3 = MyMap.count(1) + MyMap.count(2) * 2 + MyMap.count(3) / 3 >= 20;
+  // NO-WARNING.
+  // CHECK-FIXES: bool C3 = MyMap.count(1) + MyMap.count(2) * 2 + MyMap.count(3) / 3 >= 20;
+
+  // This could theoretically be rewritten into a 'contains' after removig the
+  // `4` on both sides of the comparison. For the time being, we don't detect
+  // this case.
+  bool C4 = MyMap.count(1) + 4 > 4;
+  // NO-WARNING.
+  // CHECK-FIXES: bool C4 = MyMap.count(1) + 4 > 4;
+
+  return C1 + C2 + C3 + C4;
+}
+
+// We don't want to rewrite if the `contains` call is from a macro expansion
+int testMacroExpansion(std::unordered_set<int> &MySet) {
+#define COUNT_ONES(SET) SET.count(1)
+  // Rewriting the macro would break the code
+  // CHECK-FIXES: #define COUNT_ONES(SET) SET.count(1)
+  // We still want to warn the user even if we don't offer a fixit
+  if (COUNT_ONES(MySet)) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use 'contains' to check for membership [readability-container-contains]
+    // CHECK-MESSAGES: note: expanded from macro 'COUNT_ONES'
+    return COUNT_ONES(MySet);
+  }
+#undef COUNT_ONES
+#define COUNT_ONES count(1)
+  // Rewriting the macro would break the code
+  // CHECK-FIXES: #define COUNT_ONES count(1)
+  // We still want to warn the user even if we don't offer a fixit
+  if (MySet.COUNT_ONES) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use 'contains' to check for membership [readability-container-contains]
+    // CHECK-MESSAGES: note: expanded from macro 'COUNT_ONES'
+    return MySet.COUNT_ONES;
+  }
+#undef COUNT_ONES
+#define MY_SET MySet
+  // CHECK-FIXES: #define MY_SET MySet
+  // We still want to rewrite one of the two calls to `count`
+  if (MY_SET.count(1)) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: use 'contains' to check for membership [readability-container-contains]
+    // CHECK-FIXES: if (MY_SET.contains(1)) {
+    return MY_SET.count(1);
+  }
+#undef MY_SET
+  return 0;
+}
+
+// The following map has the same interface like `std::map`.
+template <class Key, class T>
+struct CustomMap {
+  unsigned count(const Key &K) const;
+  bool contains(const Key &K) const;
+  void *find(const Key &K);
+  void *end();
+};
+
+// The clang-tidy check is currently hard-coded against the `std::` containers
+// and hence won't annotate the following instance. We might change this in the
+// future and also detect the following case.
+void *testDifferentCheckTypes(CustomMap<int, int> &MyMap) {
+  if (MyMap.count(0))
+    // NO-WARNING.
+    // CHECK-FIXES: if (MyMap.count(0))
+    return nullptr;
+  return MyMap.find(2);
+}


        


More information about the cfe-commits mailing list