[clang-tools-extra] [clang-tidy] Add performance-redundant-lookup check (PR #125420)
Balazs Benics via cfe-commits
cfe-commits at lists.llvm.org
Sun Feb 2 08:47:19 PST 2025
https://github.com/steakhal created https://github.com/llvm/llvm-project/pull/125420
Finds redundant lookups to set|map containers.
For example: `map.count(key) && map[key] < threshold`
---
I've implemented this check in the Clang Static Analyzer, but it didn't really bring much value compared to its cost.
Because of this, I decided to have a second look and implement this in clang-tidy.
As in the [comment](https://github.com/llvm/llvm-project/issues/123376#issuecomment-2628966580) people showed interest, I decided to give a final polish to the check and publish it to ask for feedback.
My experience of this checker so far on the `clang` code base was that it didn't produce many false-positives, but those are hard to avoid due to what patterns we are looking for.
Frequently the findings were actionable and useful, and already fixed a couple issue that I found: 0e62c748d440a6d12d190e951c987efe309f40d6, 9333d8fb0749b1ae2b152a1003d2ecc00027b3d5, d0a142eaea03661e8399f2c1733b93d21d55dfee, 0d21ef4e6c50c7d4d591adf7e6dbd6232e8a99c4, 65708bad579229cd7f62b8d0eaefda4bb20eb6d8, 16d4453f2f5d9554ce39507fda0f33ce9066007b.
I've considered adding some fixits, and that may be possible if we focus on some really frequent patterns, but I decided not to invest there.
I've considered techniques to limit the scope of what "lookups" should be bundled together, for example the lookups that are in close proximity, or lookups that are within the same lexical scope, or there are no mutations between the lookups; but ultimately I found any of these arbitrary and confusing - though I admit, there could be improvements.
Let me know what you think.
>From 78390b2af01fcd5acfbd2a313852962c873e5611 Mon Sep 17 00:00:00 2001
From: Balazs Benics <benicsbalazs at gmail.com>
Date: Sun, 2 Feb 2025 17:35:39 +0100
Subject: [PATCH 1/2] [clang-tidy] Add performance-redundant-lookup check
Finds redundant lookups to set|map containers.
For example: `map.count(key) && map[key] < threshold`
---
.../clang-tidy/performance/CMakeLists.txt | 1 +
.../performance/PerformanceTidyModule.cpp | 3 +
.../performance/RedundantLookupCheck.cpp | 159 ++++++++++++++++++
.../performance/RedundantLookupCheck.h | 46 +++++
clang-tools-extra/docs/ReleaseNotes.rst | 5 +
.../docs/clang-tidy/checks/list.rst | 1 +
.../checks/performance/redundant-lookup.rst | 147 ++++++++++++++++
.../checkers/performance/redundant-lookup.cpp | 142 ++++++++++++++++
8 files changed, 504 insertions(+)
create mode 100644 clang-tools-extra/clang-tidy/performance/RedundantLookupCheck.cpp
create mode 100644 clang-tools-extra/clang-tidy/performance/RedundantLookupCheck.h
create mode 100644 clang-tools-extra/docs/clang-tidy/checks/performance/redundant-lookup.rst
create mode 100644 clang-tools-extra/test/clang-tidy/checkers/performance/redundant-lookup.cpp
diff --git a/clang-tools-extra/clang-tidy/performance/CMakeLists.txt b/clang-tools-extra/clang-tidy/performance/CMakeLists.txt
index c6e547c5089fb0e..6f907852c9fd1c4 100644
--- a/clang-tools-extra/clang-tidy/performance/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/performance/CMakeLists.txt
@@ -21,6 +21,7 @@ add_clang_library(clangTidyPerformanceModule STATIC
NoexceptMoveConstructorCheck.cpp
NoexceptSwapCheck.cpp
PerformanceTidyModule.cpp
+ RedundantLookupCheck.cpp
TriviallyDestructibleCheck.cpp
TypePromotionInMathFnCheck.cpp
UnnecessaryCopyInitialization.cpp
diff --git a/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp b/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
index 9e0fa6f88b36a00..a613114e6b83bd3 100644
--- a/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
@@ -24,6 +24,7 @@
#include "NoexceptDestructorCheck.h"
#include "NoexceptMoveConstructorCheck.h"
#include "NoexceptSwapCheck.h"
+#include "RedundantLookupCheck.h"
#include "TriviallyDestructibleCheck.h"
#include "TypePromotionInMathFnCheck.h"
#include "UnnecessaryCopyInitialization.h"
@@ -62,6 +63,8 @@ class PerformanceModule : public ClangTidyModule {
"performance-noexcept-move-constructor");
CheckFactories.registerCheck<NoexceptSwapCheck>(
"performance-noexcept-swap");
+ CheckFactories.registerCheck<RedundantLookupCheck>(
+ "performance-redundant-lookup");
CheckFactories.registerCheck<TriviallyDestructibleCheck>(
"performance-trivially-destructible");
CheckFactories.registerCheck<TypePromotionInMathFnCheck>(
diff --git a/clang-tools-extra/clang-tidy/performance/RedundantLookupCheck.cpp b/clang-tools-extra/clang-tidy/performance/RedundantLookupCheck.cpp
new file mode 100644
index 000000000000000..fefb28682c5b8af
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/performance/RedundantLookupCheck.cpp
@@ -0,0 +1,159 @@
+//===--- RedundantLookupCheck.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 "RedundantLookupCheck.h"
+#include "../utils/OptionsUtils.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Regex.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::performance {
+
+static constexpr auto DefaultContainerNameRegex = "set|map";
+
+static const llvm::StringRef DefaultLookupMethodNames =
+ llvm::StringLiteral( //
+ "at;"
+ "contains;"
+ "count;"
+ "find_as;"
+ "find;"
+ // These are tricky, as they take the "key" at different places.
+ // They sometimes bundle up the key and the value together in a pair.
+ // "emplace;"
+ // "insert_or_assign;"
+ // "insert;"
+ // "try_emplace;"
+ )
+ .drop_back(); // Drops the last semicolon.
+
+RedundantLookupCheck::RedundantLookupCheck(StringRef Name,
+ ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context),
+ ContainerNameRegex(
+ Options.get("ContainerNameRegex", DefaultContainerNameRegex)),
+ LookupMethodNames(utils::options::parseStringList(
+ Options.get("LookupMethodNames", DefaultLookupMethodNames))) {}
+
+void RedundantLookupCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+ Options.store(Opts, "ContainerNameRegex", ContainerNameRegex);
+ Options.store(Opts, "LookupMethodNames",
+ utils::options::serializeStringList(LookupMethodNames));
+}
+
+namespace {
+/// Checks if any of the ends of the source range is in a macro expansion.
+AST_MATCHER(Expr, hasMacroSourceRange) {
+ SourceRange R = Node.getSourceRange();
+ return R.getBegin().isMacroID() || R.getEnd().isMacroID();
+}
+} // namespace
+
+static constexpr const char *ObjKey = "obj";
+static constexpr const char *LookupKey = "key";
+static constexpr const char *LookupCallKey = "lookup";
+static constexpr const char *EnclosingFnKey = "fn";
+
+void RedundantLookupCheck::registerMatchers(MatchFinder *Finder) {
+ auto MatchesContainerNameRegex =
+ matchesName(ContainerNameRegex, llvm::Regex::IgnoreCase);
+
+ // Match that the expression is a record type with a name that contains "map"
+ // or "set".
+ auto RecordCalledMapOrSet =
+ expr(ignoringImpCasts(hasType(hasUnqualifiedDesugaredType(recordType(
+ hasDeclaration(namedDecl(MatchesContainerNameRegex)))))))
+ .bind(ObjKey);
+
+ auto SubscriptCall =
+ cxxOperatorCallExpr(hasOverloadedOperatorName("[]"), argumentCountIs(2),
+ hasArgument(0, RecordCalledMapOrSet),
+ hasArgument(1, expr().bind(LookupKey)));
+
+ auto LookupMethodCalls =
+ cxxMemberCallExpr(on(RecordCalledMapOrSet), argumentCountIs(1),
+ hasArgument(0, expr().bind(LookupKey)),
+ callee(cxxMethodDecl(hasAnyName(LookupMethodNames))));
+
+ // Match any lookup or subscript calls that are not in a macro expansion.
+ auto AnyLookup = callExpr(unless(hasMacroSourceRange()),
+ anyOf(SubscriptCall, LookupMethodCalls))
+ .bind(LookupCallKey);
+
+ // We need to collect all lookups in a function to be able to report them in
+ // batches.
+ Finder->addMatcher(
+ functionDecl(hasBody(compoundStmt(forEachDescendant(AnyLookup))))
+ .bind(EnclosingFnKey),
+ this);
+}
+
+/// Hash the container object expr along with the key used for lookup and the
+/// enclosing function together.
+static unsigned hashLookupEvent(const ASTContext &Ctx,
+ const FunctionDecl *EnclosingFn,
+ const Expr *LookupKey,
+ const Expr *ContainerObject) {
+ llvm::FoldingSetNodeID ID;
+ ID.AddPointer(EnclosingFn);
+
+ LookupKey->Profile(ID, Ctx, /*Canonical=*/true,
+ /*ProfileLambdaExpr=*/true);
+ ContainerObject->Profile(ID, Ctx, /*Canonical=*/true,
+ /*ProfileLambdaExpr=*/true);
+ return ID.ComputeHash();
+}
+
+void RedundantLookupCheck::check(const MatchFinder::MatchResult &Result) {
+ SM = Result.SourceManager;
+
+ const auto *EnclosingFn =
+ Result.Nodes.getNodeAs<FunctionDecl>(EnclosingFnKey);
+ const auto *LookupCall = Result.Nodes.getNodeAs<CallExpr>(LookupCallKey);
+ const auto *Key = Result.Nodes.getNodeAs<Expr>(LookupKey);
+ const auto *ContainerObject = Result.Nodes.getNodeAs<Expr>(ObjKey);
+
+ unsigned LookupHash =
+ hashLookupEvent(*Result.Context, EnclosingFn, ContainerObject, Key);
+ auto &Lookups = RegisteredLookups.try_emplace(LookupHash).first->second;
+ if (!llvm::is_contained(Lookups, LookupCall))
+ Lookups.push_back(LookupCall);
+}
+
+void RedundantLookupCheck::onEndOfTranslationUnit() {
+ auto ByBeginLoc = [this](const CallExpr *Lookup1, const CallExpr *Lookup2) {
+ return SM->isBeforeInTranslationUnit(Lookup1->getBeginLoc(),
+ Lookup2->getBeginLoc());
+ };
+
+ // Process the found lookups of each function.
+ for (auto &LookupGroup : llvm::make_second_range(RegisteredLookups)) {
+ if (LookupGroup.size() < 2)
+ continue;
+
+ llvm::sort(LookupGroup, ByBeginLoc);
+
+ const CallExpr *FirstLookupCall = LookupGroup.front();
+ diag(FirstLookupCall->getBeginLoc(), "possibly redundant container lookups")
+ << FirstLookupCall->getSourceRange();
+
+ for (const CallExpr *LookupCall : llvm::drop_begin(LookupGroup)) {
+ diag(LookupCall->getBeginLoc(), "next lookup here", DiagnosticIDs::Note)
+ << LookupCall->getSourceRange();
+ }
+ }
+
+ RegisteredLookups.clear();
+}
+
+} // namespace clang::tidy::performance
diff --git a/clang-tools-extra/clang-tidy/performance/RedundantLookupCheck.h b/clang-tools-extra/clang-tidy/performance/RedundantLookupCheck.h
new file mode 100644
index 000000000000000..b37e1c2234c7673
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/performance/RedundantLookupCheck.h
@@ -0,0 +1,46 @@
+//===--- RedundantLookupCheck.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_PERFORMANCE_REDUNDANTLOOKUPCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_REDUNDANTLOOKUPCHECK_H
+
+#include "../ClangTidyCheck.h"
+#include "clang/AST/Expr.h"
+
+namespace clang {
+class SourceManager;
+} // namespace clang
+
+namespace clang::tidy::performance {
+
+/// Detects redundant container lookups.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/performance/redundant-lookup.html
+class RedundantLookupCheck : public ClangTidyCheck {
+public:
+ RedundantLookupCheck(StringRef Name, ClangTidyContext *Context);
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+ void onEndOfTranslationUnit() override;
+ void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+ bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+ return LangOpts.CPlusPlus;
+ }
+
+private:
+ llvm::DenseMap<unsigned, llvm::SmallVector<const CallExpr *>>
+ RegisteredLookups;
+ const StringRef ContainerNameRegex;
+ const std::vector<StringRef> LookupMethodNames;
+ const SourceManager *SM = nullptr;
+};
+
+} // namespace clang::tidy::performance
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_REDUNDANTLOOKUPCHECK_H
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 3bddeeda06e06b9..2287e9f5caaf953 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -91,6 +91,11 @@ Improvements to clang-tidy
New checks
^^^^^^^^^^
+- New :doc:`performance-redundant-lookup
+ <clang-tidy/checks/performance/redundant-lookup>` check.
+
+ Detects redundant container lookups.
+
New check aliases
^^^^^^^^^^^^^^^^^
diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst
index 7b9b905ef76715e..43f3d147c4c3099 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -344,6 +344,7 @@ Clang-Tidy Checks
:doc:`performance-noexcept-destructor <performance/noexcept-destructor>`, "Yes"
:doc:`performance-noexcept-move-constructor <performance/noexcept-move-constructor>`, "Yes"
:doc:`performance-noexcept-swap <performance/noexcept-swap>`, "Yes"
+ :doc:`performance-redundant-lookup <performance/redundant-lookup>`, "Yes"
:doc:`performance-trivially-destructible <performance/trivially-destructible>`, "Yes"
:doc:`performance-type-promotion-in-math-fn <performance/type-promotion-in-math-fn>`, "Yes"
:doc:`performance-unnecessary-copy-initialization <performance/unnecessary-copy-initialization>`, "Yes"
diff --git a/clang-tools-extra/docs/clang-tidy/checks/performance/redundant-lookup.rst b/clang-tools-extra/docs/clang-tidy/checks/performance/redundant-lookup.rst
new file mode 100644
index 000000000000000..6f25215e867ead8
--- /dev/null
+++ b/clang-tools-extra/docs/clang-tidy/checks/performance/redundant-lookup.rst
@@ -0,0 +1,147 @@
+.. title:: clang-tidy - performance-redundant-lookup
+
+performance-redundant-lookup
+============================
+
+This check warns about potential redundant container lookup operations within
+a function.
+
+Examples
+--------
+
+.. code-block:: c++
+
+ if (map.count(key) && map[key] < threshold) {
+ // do stuff
+ }
+
+In this example, we would check if the key is present in the map,
+and then do a second lookup to actually load the value.
+We could refactor the code into this, to use a single lookup:
+
+.. code-block:: c++
+
+ if (auto it = map.find(key); it != map.end() && it->second < threshold) {
+ // do stuff
+ }
+
+In this example, we do three lookups while calculating, caching and then
+using the result of some expensive computation:
+
+.. code-block:: c++
+
+ if (!cache.contains(key)) {
+ cache[key] = computeExpensiveValue();
+ }
+ use(cache[key]);
+
+We could refactor this code using ``try_emplace`` to fill up the cache entry
+if wasn't present, and just use it if we already computed the value.
+This way we would only have a single unavoidable lookup:
+
+.. code-block:: c++
+
+ auto [cacheSlot, inserted] cache.try_emplace(key);
+ if (inserted) {
+ cacheSlot->second = computeExpensiveValue();
+ }
+ use(cacheSlot->second);
+
+
+What is a "lookup"?
+-------------------
+
+All container operations that walk the internal structure of the container
+should be considered as "lookups".
+This means that checking if an element is present or inserting an element
+is also considered as a "lookup".
+
+For example, ``contains``, ``count`` but even the ``operator[]``
+should be considered as "lookups".
+
+Technically ``insert``, ``emplace`` or ``try_emplace``are also lookups,
+even if due to limitations, they are not recognized as such.
+
+Lookups inside macros are ignored, thus not considered as "lookups".
+For example:
+
+.. code-block:: c++
+
+ assert(map.count(key) == 0); // Not considered as a "lookup".
+
+Limitations
+-----------
+
+ - The "redundant lookups" may span across a large chunk of code.
+ Such reports can be considered as false-positives because it's hard to judge
+ if the container is definitely not mutated between the lookups.
+ It would be hard to split the lookup groups in a stable and meaningful way,
+ and a threshold for proximity would be just an arbitrary limit.
+
+ - The "redundant lookups" may span across different control-flow constructs,
+ making it impossible to refactor.
+ It may be that the code was deliberately structured like it was, thus the
+ report is considered a false-positive.
+ Use your best judgement to see if anything needs to be fixed or not.
+ For example:
+
+ .. code-block:: c++
+
+ if (coin())
+ map[key] = foo();
+ else
+ map[key] = bar();
+
+ Could be refactored into:
+
+ .. code-block:: c++
+
+ map[key] = coin() ? foo() : bar();
+
+ However, the following code could be considered intentional:
+
+ .. code-block:: c++
+
+ // Handle the likely case.
+ if (auto it = map.find(key); it != map.end()) {
+ return process(*it);
+ }
+
+ // Commit the dirty items, and check again.
+ for (const auto &item : dirtyList) {
+ commit(item, map); // Updates the "map".
+ }
+
+ // Do a final check.
+ if (auto it = map.find(key); it != map.end()) {
+ return process(*it);
+ }
+
+ - The key argument of a lookup may have sideffects. Sideffects are ignored when identifying lookups.
+ This can introduce some false-positives. For example:
+
+ .. code-block:: c++
+
+ m.contains(rng(++n));
+ m.contains(rng(++n)); // FP: This is considered a redundant lookup.
+
+ - Lookup member functions must have exactly 1 argument to match.
+ There are technically lookup functions, such as ``insert`` or ``try_emplace``,
+ but it would be hard to identify the "key" part of the argument,
+ while leaving the implementation open for user-configuration via the
+ ``LookupMethodNames`` option.
+
+Options
+-------
+
+.. option:: ContainerNameRegex
+
+ The regular expression matching the type of the container objects.
+ This is matched in a case insensitive manner.
+ Default is ``set|map``.
+
+.. option:: LookupMethodNames
+
+ Member function names to consider as **lookup** operation.
+ These methods must have exactly 1 argument.
+ Default is ``at;contains;count;find_as;find``.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance/redundant-lookup.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance/redundant-lookup.cpp
new file mode 100644
index 000000000000000..c686feca7f44c8e
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/performance/redundant-lookup.cpp
@@ -0,0 +1,142 @@
+// RUN: %check_clang_tidy %s performance-redundant-lookup %t -- -- -isystem %clang_tidy_headers
+
+#include <map>
+#include <set>
+
+#define myassert(cond) do { if (!(cond)) myabort(); } while (0)
+void myabort() __attribute__((noreturn));
+int rng(int seed);
+template <class T, class... Ts> void escape(T&, Ts&...);
+
+namespace my {
+template <class T, class U> class FancyMap {
+public:
+ using key_type = T;
+ bool contains(key_type) const;
+ int count(key_type) const;
+ U &operator[](key_type);
+ const U &operator[](key_type) const;
+
+private:
+ key_type* keys;
+ U* values;
+};
+
+template <class T> class FancySet {
+public:
+ using key_type = T;
+ bool contains(key_type) const;
+ int count(key_type) const;
+
+private:
+ key_type* keys;
+};
+} // namespace my
+
+
+void containerNameSpellsSet(my::FancySet<int> &s, int key) {
+ (void)s.contains(key); // CHECK-MESSAGES: :[[@LINE]]:9: warning: possibly redundant container lookups [performance-redundant-lookup]
+ (void)s.contains(key); // CHECK-MESSAGES: :[[@LINE]]:9: note: next lookup here
+}
+
+void containerNameSpellsMap(my::FancyMap<int, int> &s, int key) {
+ (void)s.count(key); // CHECK-MESSAGES: :[[@LINE]]:9: warning: possibly redundant container lookups [performance-redundant-lookup]
+ (void)s.count(key); // CHECK-MESSAGES: :[[@LINE]]:9: note: next lookup here
+}
+
+void stdSetAlsoWorks(std::set<int> &s, int key) {
+ (void)s.count(key); // CHECK-MESSAGES: :[[@LINE]]:9: warning: possibly redundant container lookups [performance-redundant-lookup]
+ (void)s.count(key); // CHECK-MESSAGES: :[[@LINE]]:9: note: next lookup here
+}
+
+void stdMapAlsoWorks(std::map<int, int> &m, int key) {
+ (void)m.count(key); // CHECK-MESSAGES: :[[@LINE]]:9: warning: possibly redundant container lookups [performance-redundant-lookup]
+ (void)m.count(key); // CHECK-MESSAGES: :[[@LINE]]:9: note: next lookup here
+}
+
+void differentLookupKeys(std::map<int, int> &m, int key, int key2, int val) {
+ (void)m.contains(key);
+ m[key2] = val; // no-warning: The second lookup uses a different key.
+}
+
+void differentContainers(std::map<int, int> &first, std::map<int, int> &second, int key, int val) {
+ (void)first.contains(key);
+ second[key] = val; // no-warning: The second lookup is on a different container.
+}
+
+void countThenContains(std::map<int, int> &m, int key) {
+ (void)m.count(key); // CHECK-MESSAGES: :[[@LINE]]:9: warning: possibly redundant container lookups
+ (void)m.contains(key); // CHECK-MESSAGES: :[[@LINE]]:9: note: next lookup here
+}
+
+void containsThanContains(std::map<int, int> &m, int key) {
+ (void)m.contains(key); // CHECK-MESSAGES: :[[@LINE]]:9: warning: possibly redundant container lookups
+ (void)m.contains(key); // CHECK-MESSAGES: :[[@LINE]]:9: note: next lookup here
+}
+
+void subscriptThenContains(std::map<int, int> &m, int key) {
+ (void)m[key]; // CHECK-MESSAGES: :[[@LINE]]:9: warning: possibly redundant container lookups
+ (void)m.contains(key); // CHECK-MESSAGES: :[[@LINE]]:9: note: next lookup here
+}
+
+void allLookupsAreMentioned(std::map<int, int> &m, int key) {
+ (void)m.at(key); // CHECK-MESSAGES: :[[@LINE]]:9: warning: possibly redundant container lookups
+ (void)m.contains(key); // CHECK-MESSAGES: :[[@LINE]]:9: note: next lookup here
+ (void)m.count(key); // CHECK-MESSAGES: :[[@LINE]]:9: note: next lookup here
+ (void)m.find(key); // CHECK-MESSAGES: :[[@LINE]]:9: note: next lookup here
+ (void)m.size(); // no-warning: Not a lookup call.
+ m[key] = 1; // CHECK-MESSAGES: :[[@LINE]]:3: note: next lookup here
+ (void)m[key]; // CHECK-MESSAGES: :[[@LINE]]:9: note: next lookup here
+ (void)m.count(key); // CHECK-MESSAGES: :[[@LINE]]:9: note: next lookup here
+ (void)m.emplace(key, 2);
+ (void)m.emplace(key, 2);
+ (void)m.try_emplace(key, 3);
+ (void)m.try_emplace(key, 3);
+ (void)m.insert({key, 4});
+ (void)m.insert({key, 4});
+}
+
+void aliasesAreNotTracked(std::map<int, int> &m, int key) {
+ auto &alias = m;
+ (void)alias[key];
+ (void)m.contains(key); // FIXME: "alias" is considered a separate object.
+}
+
+void mutationBetweenLookups(std::map<int, int> &m, int key) {
+ extern std::map<int, int> *global_map;
+ // FP: We ignore mutations between the lookups.
+ (void)m.contains(key); // CHECK-MESSAGES: :[[@LINE]]:9: warning: possibly redundant container lookups
+ global_map = &m;
+ escape(m);
+ (void)m.contains(key); // CHECK-MESSAGES: :[[@LINE]]:9: note: next lookup here
+}
+
+void nested(std::map<int, int> &m, int key) {
+ (void)m.contains(key); // no-warning: This lookup is in a different stack frame.
+}
+void onlyLocalLookupsAreConsidered(std::map<int, int> &m, int key) {
+ (void)m.contains(key);
+ nested(m, key);
+}
+
+void lookupsWithinMacrosAreIgnored(std::map<int, int> &m, int key) {
+ myassert(m.count(key) == 0);
+ myassert(m.count(key) == 0);
+ myassert(m.count(key) == 0);
+ (void)m.count(key); // no-warining: This is just the first lookup that counts.
+}
+
+void lookupsWithinMacrosAreIgnored2(std::map<int, int> &m, int key) {
+ (void)m.count(key); // CHECK-MESSAGES: :[[@LINE]]:9: warning: possibly redundant container lookups
+ myassert(m.count(key) == 0);
+ myassert(m.count(key) == 0);
+ myassert(m.count(key) == 0);
+ m[key] = 10; // CHECK-MESSAGES: :[[@LINE]]:3: note: next lookup here
+}
+
+void sideffectsAreIgnoredInKeyExpr(std::map<int, int> &m, int n) {
+ // FIXME: This is a FP. We should probably ignore expressions with definite sideffects.
+ (void)m.contains(rng(++n)); // CHECK-MESSAGES: :[[@LINE]]:9: warning: possibly redundant container lookups
+ (void)m.contains(rng(++n)); // CHECK-MESSAGES: :[[@LINE]]:9: note: next lookup here
+ (void)m.contains(rng(n + 1)); // no-warning: This uses a different lookup key.
+}
>From ae03a452b0bf8ffc3250415943df3d0b79adb3d9 Mon Sep 17 00:00:00 2001
From: Balazs Benics <benicsbalazs at gmail.com>
Date: Sun, 2 Feb 2025 17:35:39 +0100
Subject: [PATCH 2/2] [clang-tidy][NFC] Mock set, map, memory, utility and
functional headers
Mock functional: std::less
Mock set, map
Mock memory: std::allocator
Mock utility: std::pair
---
.../checkers/Inputs/Headers/functional | 21 +++
.../clang-tidy/checkers/Inputs/Headers/map | 167 ++++++++++++++++++
.../clang-tidy/checkers/Inputs/Headers/memory | 10 ++
.../clang-tidy/checkers/Inputs/Headers/set | 145 +++++++++++++++
.../checkers/Inputs/Headers/utility | 13 ++
5 files changed, 356 insertions(+)
create mode 100644 clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/functional
create mode 100644 clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/map
create mode 100644 clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/memory
create mode 100644 clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/set
create mode 100644 clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/utility
diff --git a/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/functional b/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/functional
new file mode 100644
index 000000000000000..be845b3c7e68a76
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/functional
@@ -0,0 +1,21 @@
+#ifndef _FUNCTIONAL_
+#define _FUNCTIONAL_
+
+namespace std {
+
+template <class T> struct less {
+ constexpr bool operator()(const T& Lhs, const T& Rhs) const {
+ return Lhs < Rhs;
+ }
+};
+
+template <> struct less<void> {
+ template <typename T, typename U>
+ constexpr bool operator()(T &&Lhs, U &&Rhs) const {
+ return static_cast<T &&>(Lhs) < static_cast<U &&>(Rhs);
+ }
+};
+
+} // namespace std
+
+#endif // _FUNCTIONAL_
diff --git a/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/map b/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/map
new file mode 100644
index 000000000000000..1c7b26c93f7270d
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/map
@@ -0,0 +1,167 @@
+#ifndef _MAP_
+#define _MAP_
+
+#include <functional> // std::less
+#include <memory> // std::allocator
+#include <utility> // std::pair
+
+namespace std {
+
+using size_t = decltype(sizeof(0));
+using ptrdiff_t = decltype(static_cast<int*>(nullptr) - static_cast<int*>(nullptr));
+
+template<
+ class Key,
+ class T,
+ class Compare = std::less<Key>,
+ class Allocator = std::allocator<std::pair<const Key, T>>
+> class map {
+public:
+ using key_type = Key;
+ using mapped_type = T;
+ using value_type = std::pair<const Key, T>;
+ using size_type = size_t;
+ using difference_type = ptrdiff_t;
+ using key_compare = Compare;
+ using allocator_type = Allocator;
+ using reference = value_type&;
+ using const_reference = const value_type&;
+
+ map();
+ explicit map(const Allocator&);
+ template <class InputIt>
+ map(InputIt, InputIt, const Compare& = Compare(), const Allocator& = Allocator());
+ template <class InputIt> map(InputIt, InputIt, const Allocator&);
+ map(const map&, const Allocator&);
+ map(map&&, const Allocator&);
+ // missing: init-list ctors
+ // missing: from_range ctors
+
+ ~map();
+
+ map& operator=(const map&);
+ map& operator=(map&&);
+
+ T& at(const Key&);
+ const T& at(const Key&) const;
+ template <class K> T& at(const K&); // C++26
+ template <class K> const T& at(const K&) const; // C++26
+
+ T& operator[](const Key&);
+ T& operator[](Key&&);
+ template <class K> T& operator[](K&&); // C++26
+
+ struct iterator {};
+ struct const_iterator {};
+
+ iterator begin();
+ const_iterator begin() const;
+ const_iterator cbegin() const noexcept;
+
+ iterator end();
+ const_iterator end() const;
+ const_iterator cend() const noexcept;
+
+ iterator rbegin();
+ const_iterator rbegin() const;
+ const_iterator crbegin() const noexcept;
+
+ iterator rend();
+ const_iterator rend() const;
+ const_iterator crend() const noexcept;
+
+ bool empty() const;
+ size_type size() const;
+ size_type max_size() const;
+ void clear();
+
+ std::pair<iterator, bool> insert(const value_type&);
+ template <class P> std::pair<iterator, bool> insert(P&&);
+ std::pair<iterator, bool> insert(value_type&&);
+ iterator insert(const_iterator, const value_type&);
+ iterator insert(const_iterator, value_type&&);
+ template <class InputIt> void insert(InputIt, InputIt);
+ // void insert(std::initializer_list<value_type>);
+ // insert_return_type insert(node_type&&);
+ // iterator insert(const_iterator, node_type&&);
+
+ template <class R> void insert_range(R&&); // C++23
+
+ template <class M> std::pair<iterator, bool> insert_or_assign(const Key&, M&&);
+ template <class M> std::pair<iterator, bool> insert_or_assign(Key&&, M&&);
+ template <class K, class M> std::pair<iterator, bool> insert_or_assign(K&&, M&&); // C++26
+ template <class M> iterator insert_or_assign(const_iterator, const Key&, M&&);
+ template <class M> iterator insert_or_assign(const_iterator, Key&&, M&&);
+ template <class K, class M> iterator insert_or_assign(const_iterator, K&&, M&&); // C++26
+
+ template <class... Args> std::pair<iterator, bool> emplace(Args&&...);
+ template <class... Args> iterator emplace_hint(const_iterator, Args&&...);
+
+ template <class... Args> std::pair<iterator, bool> try_emplace(const Key&, Args&&...);
+ template <class... Args> std::pair<iterator, bool> try_emplace(Key&&, Args&&...);
+ template <class K, class... Args> std::pair<iterator, bool> try_emplace(K&&, Args&&...); // C++26
+ template <class... Args> iterator try_emplace(const_iterator, const Key&, Args&&...);
+ template <class... Args> iterator try_emplace(const_iterator, Key&&, Args&&...);
+ template <class K, class... Args> iterator try_emplace(const_iterator, K&&, Args&&...); // C++26
+
+ iterator erase(iterator);
+ iterator erase(const_iterator);
+ iterator erase(const_iterator, const_iterator);
+ size_type erase(const Key&);
+ template <class K> size_type erase(K&&);
+
+ void swap(map&);
+
+ // extract
+ // merge
+
+ size_type count(const Key&) const;
+ template <class K> size_type count(const K&) const;
+
+ iterator find(const Key&);
+ const_iterator find(const Key&) const;
+ template <class K> iterator find(const K&);
+ template <class K> const_iterator find(const K&) const;
+
+ bool contains(const Key&) const;
+ template <class K> bool contains(const K&) const;
+
+ std::pair<iterator, iterator> equal_range(const Key&);
+ std::pair<const_iterator, const_iterator> equal_range(const Key&) const;
+ template <class K> std::pair<iterator, iterator> equal_range(const K&);
+ template <class K> std::pair<const_iterator, const_iterator> equal_range(const K&) const;
+
+ iterator lower_bound(const Key&);
+ const_iterator lower_bound(const Key&) const;
+ template <class K> iterator lower_bound(const K&);
+ template <class K> const_iterator lower_bound(const K&) const;
+
+ iterator upper_bound(const Key&);
+ const_iterator upper_bound(const Key&) const;
+ template <class K> iterator upper_bound(const K&);
+ template <class K> const_iterator upper_bound(const K&) const;
+
+ template <class Key2, class T2, class Compare2, class Alloc2>
+ friend bool operator==(const std::map<Key2, T2, Compare2, Alloc2>&,
+ const std::map<Key2, T2, Compare2, Alloc2>&);
+ template <class Key2, class T2, class Compare2, class Alloc2>
+ friend bool operator!=(const std::map<Key2, T2, Compare2, Alloc2>&,
+ const std::map<Key2, T2, Compare2, Alloc2>&);
+ template <class Key2, class T2, class Compare2, class Alloc2>
+ friend bool operator<(const std::map<Key2, T2, Compare2, Alloc2>&,
+ const std::map<Key2, T2, Compare2, Alloc2>&);
+ template <class Key2, class T2, class Compare2, class Alloc2>
+ friend bool operator<=(const std::map<Key2, T2, Compare2, Alloc2>&,
+ const std::map<Key2, T2, Compare2, Alloc2>&);
+ template <class Key2, class T2, class Compare2, class Alloc2>
+ friend bool operator>(const std::map<Key2, T2, Compare2, Alloc2>&,
+ const std::map<Key2, T2, Compare2, Alloc2>&);
+ template <class Key2, class T2, class Compare2, class Alloc2>
+ friend bool operator>=(const std::map<Key2, T2, Compare2, Alloc2>&,
+ const std::map<Key2, T2, Compare2, Alloc2>&);
+ // operator<=>
+};
+
+} // namespace std
+
+#endif // _MAP_
diff --git a/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/memory b/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/memory
new file mode 100644
index 000000000000000..74e0005afef6335
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/memory
@@ -0,0 +1,10 @@
+#ifndef _MEMORY_
+#define _MEMORY_
+
+namespace std {
+
+template <typename T> class allocator {};
+
+} // namespace std
+
+#endif // _MEMORY_
diff --git a/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/set b/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/set
new file mode 100644
index 000000000000000..c4eb9f538cdb196
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/set
@@ -0,0 +1,145 @@
+#ifndef _SET_
+#define _SET_
+
+#include <functional> // std::less
+#include <memory> // std::allocator
+#include <utility> // std::pair
+
+namespace std {
+
+using size_t = decltype(sizeof(0));
+using ptrdiff_t = decltype(static_cast<int*>(nullptr) - static_cast<int*>(nullptr));
+
+template<
+ class Key,
+ class Compare = std::less<Key>,
+ class Allocator = std::allocator<Key>
+> class set {
+public:
+ using key_type = Key;
+ using value_type = Key;
+ using size_type = size_t;
+ using difference_type = ptrdiff_t;
+ using key_compare = Compare;
+ using value_compare = Compare;
+ using allocator_type = Allocator;
+ using reference = value_type&;
+ using const_reference = const value_type&;
+
+ set();
+ explicit set(const Allocator&);
+ template <class InputIt>
+ set(InputIt, InputIt, const Compare& = Compare(), const Allocator& = Allocator() );
+ template <class InputIt> set(InputIt, InputIt, const Allocator&);
+ set(const set&);
+ set(const set&, const Allocator&);
+ set(set&&);
+ set(set&&, const Allocator&);
+ // missing: init-list ctors
+ // missing: from_range ctors
+
+ ~set();
+
+ set& operator=(const set&);
+ set& operator=(set&&);
+
+ struct iterator{};
+ struct const_iterator{};
+
+ iterator begin();
+ const_iterator begin() const;
+ const_iterator cbegin() const noexcept;
+
+ iterator end();
+ const_iterator end() const;
+ const_iterator cend() const noexcept;
+
+ iterator rbegin();
+ const_iterator rbegin() const;
+ const_iterator crbegin() const noexcept;
+
+ iterator rend();
+ const_iterator rend() const;
+ const_iterator crend() const noexcept;
+
+ bool empty() const;
+ size_type size() const;
+ size_type max_size() const;
+ void clear();
+
+ std::pair<iterator, bool> insert(const value_type&);
+ std::pair<iterator, bool> insert(value_type&&);
+ iterator insert(const_iterator, const value_type&);
+ iterator insert(const_iterator, value_type&&);
+ template <class InputIt> void insert(InputIt, InputIt);
+ // void insert(std::initializer_list<value_type>);
+ // insert_return_type insert(node_type&&);
+ // iterator insert(const_iterator, node_type&&);
+ template <class K> std::pair<iterator, bool> insert(K&&);
+ template <class K> iterator insert(const_iterator, K&&);
+
+ template <class R> void insert_range(R&&);
+
+ template <class... Args> std::pair<iterator, bool> emplace(Args&&...);
+
+ template <class... Args> iterator emplace_hint(const_iterator, Args&&...);
+
+ iterator erase(iterator);
+ //iterator erase(iterator) requires(!std::same_as<iterator, const_iterator>); // C++23
+ iterator erase(const_iterator);
+ iterator erase(const_iterator, const_iterator);
+ size_type erase(const Key&);
+ template <class K> size_type erase(K&&);
+
+ void swap(set&);
+
+ // extract
+ // merge
+
+ size_type count(const Key&) const;
+ template <class K> size_type count(const K&) const;
+
+ iterator find(const Key&);
+ const_iterator find(const Key&) const;
+ template <class K> iterator find(const K&);
+ template <class K> const_iterator find(const K&) const;
+
+ bool contains(const Key&) const;
+ template <class K> bool contains(const K&) const;
+ std::pair<iterator, iterator> equal_range(const Key&);
+ std::pair<const_iterator, const_iterator> equal_range(const Key&) const;
+ template <class K> std::pair<iterator, iterator> equal_range(const K&);
+ template <class K> std::pair<const_iterator, const_iterator> equal_range(const K&) const;
+
+ iterator lower_bound(const Key&);
+ const_iterator lower_bound(const Key&) const;
+ template <class K> iterator lower_bound(const K&);
+ template <class K> const_iterator lower_bound(const K&) const;
+ iterator upper_bound(const Key&);
+ const_iterator upper_bound(const Key&) const;
+ template <class K> iterator upper_bound(const K&);
+ template <class K> const_iterator upper_bound(const K&) const;
+
+ template <class Key2, class Compare2, class Alloc2>
+ friend bool operator==(const std::set<Key2, Compare2, Alloc2>&,
+ const std::set<Key2, Compare2, Alloc2>&);
+ template <class Key2, class Compare2, class Alloc2>
+ friend bool operator!=(const std::set<Key2, Compare2, Alloc2>&,
+ const std::set<Key2, Compare2, Alloc2>&);
+ template <class Key2, class Compare2, class Alloc2>
+ friend bool operator<(const std::set<Key2, Compare2, Alloc2>&,
+ const std::set<Key2, Compare2, Alloc2>&);
+ template <class Key2, class Compare2, class Alloc2>
+ friend bool operator<=(const std::set<Key2, Compare2, Alloc2>&,
+ const std::set<Key2, Compare2, Alloc2>&);
+ template <class Key2, class Compare2, class Alloc2>
+ friend bool operator>(const std::set<Key2, Compare2, Alloc2>&,
+ const std::set<Key2, Compare2, Alloc2>&);
+ template <class Key2, class Compare2, class Alloc2>
+ friend bool operator>=(const std::set<Key2, Compare2, Alloc2>&,
+ const std::set<Key2, Compare2, Alloc2>&);
+};
+
+} // namespace std
+
+#endif // _SET_
diff --git a/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/utility b/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/utility
new file mode 100644
index 000000000000000..9af8c4493f57918
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/utility
@@ -0,0 +1,13 @@
+#ifndef _UTILITY_
+#define _UTILITY_
+
+namespace std {
+
+template <class T1, class T2> struct pair {
+ T1 first;
+ T1 second;
+};
+
+} // namespace std
+
+#endif // _UTILITY_
More information about the cfe-commits
mailing list