[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:59:01 PST 2025
https://github.com/steakhal updated https://github.com/llvm/llvm-project/pull/125420
>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/4] [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/4] [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_
>From 09b6c505ff963e9e8da95a2b8f0f4383b3ff760e Mon Sep 17 00:00:00 2001
From: Balazs Benics <benicsbalazs at gmail.com>
Date: Sun, 2 Feb 2025 17:56:55 +0100
Subject: [PATCH 3/4] Fix doc syntax
---
.../docs/clang-tidy/checks/performance/redundant-lookup.rst | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
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
index 6f25215e867ead8..a5ae0f543139eea 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/performance/redundant-lookup.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/performance/redundant-lookup.rst
@@ -59,7 +59,7 @@ 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,
+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".
>From 67cbabf6bfba7064c0b0c58a99c3e843b61d973b Mon Sep 17 00:00:00 2001
From: Balazs Benics <benicsbalazs at gmail.com>
Date: Sun, 2 Feb 2025 17:57:23 +0100
Subject: [PATCH 4/4] Swap the Limitations and Options sections
---
.../checks/performance/redundant-lookup.rst | 30 +++++++++----------
1 file changed, 15 insertions(+), 15 deletions(-)
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
index a5ae0f543139eea..0366588cc7ad23e 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/performance/redundant-lookup.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/performance/redundant-lookup.rst
@@ -69,6 +69,21 @@ For example:
assert(map.count(key) == 0); // Not considered as a "lookup".
+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``.
+
Limitations
-----------
@@ -130,18 +145,3 @@ Limitations
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``.
More information about the cfe-commits
mailing list