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 <benicsbal...@gmail.com> Date: Sun, 2 Feb 2025 17:35:39 +0100 Subject: [PATCH 1/6] [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 <benicsbal...@gmail.com> Date: Sun, 2 Feb 2025 17:35:39 +0100 Subject: [PATCH 2/6] [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 <benicsbal...@gmail.com> Date: Sun, 2 Feb 2025 17:56:55 +0100 Subject: [PATCH 3/6] 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 <benicsbal...@gmail.com> Date: Sun, 2 Feb 2025 17:57:23 +0100 Subject: [PATCH 4/6] 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``. >From 691da4ab2de100ac19895198b2eed9216a620b5c Mon Sep 17 00:00:00 2001 From: Balazs Benics <benicsbal...@gmail.com> Date: Sun, 2 Feb 2025 18:10:26 +0100 Subject: [PATCH 5/6] Apply suggestions from code review Co-authored-by: EugeneZelenko <eugene.zele...@gmail.com> --- .../clang-tidy/performance/RedundantLookupCheck.cpp | 2 +- .../docs/clang-tidy/checks/performance/redundant-lookup.rst | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/clang-tools-extra/clang-tidy/performance/RedundantLookupCheck.cpp b/clang-tools-extra/clang-tidy/performance/RedundantLookupCheck.cpp index fefb28682c5b8af..7ee25ea8bf59395 100644 --- a/clang-tools-extra/clang-tidy/performance/RedundantLookupCheck.cpp +++ b/clang-tools-extra/clang-tidy/performance/RedundantLookupCheck.cpp @@ -123,7 +123,7 @@ void RedundantLookupCheck::check(const MatchFinder::MatchResult &Result) { const auto *Key = Result.Nodes.getNodeAs<Expr>(LookupKey); const auto *ContainerObject = Result.Nodes.getNodeAs<Expr>(ObjKey); - unsigned LookupHash = + const unsigned LookupHash = hashLookupEvent(*Result.Context, EnclosingFn, ContainerObject, Key); auto &Lookups = RegisteredLookups.try_emplace(LookupHash).first->second; if (!llvm::is_contained(Lookups, LookupCall)) 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 0366588cc7ad23e..46d85f1717219dc 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 @@ -76,7 +76,7 @@ Options The regular expression matching the type of the container objects. This is matched in a case insensitive manner. - Default is ``set|map``. + Default is `set|map`. .. option:: LookupMethodNames @@ -144,4 +144,4 @@ Limitations 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. + `LookupMethodNames` option. >From 7aadfd97b74e78043f8f13dc87d323d11b1d9cde Mon Sep 17 00:00:00 2001 From: Balazs Benics <benicsbal...@gmail.com> Date: Sun, 2 Feb 2025 18:11:33 +0100 Subject: [PATCH 6/6] Apply suggestions from code review Co-authored-by: EugeneZelenko <eugene.zele...@gmail.com> --- .../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 46d85f1717219dc..64e9f7cbe8d7b56 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 @@ -82,7 +82,7 @@ Options Member function names to consider as **lookup** operation. These methods must have exactly 1 argument. - Default is ``at;contains;count;find_as;find``. + Default is `at;contains;count;find_as;find`. Limitations ----------- _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits