[clang-tidy] Add performance-redundant-lookup check #125420
Conversation
Finds redundant lookups to set|map containers. For example: `map.count(key) && map[key] < threshold`
Mock functional: std::less Mock set, map Mock memory: std::allocator Mock utility: std::pair
steakhal
requested review from
PiotrZSL,
5chmidti,
kazutakahirata and
HerrCai0907
|
@llvm/pr-subscribers-clang-tidy Author: Balazs Benics (steakhal) ChangesFinds redundant lookups to set|map containers. I've implemented this check in the Clang Static Analyzer, but it didn't really bring much value compared to its cost. My experience of this checker so far on the 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. Let me know what you think. Patch is 35.66 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/125420.diff 13 Files Affected:
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/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 ...
[truncated]
|
|
@llvm/pr-subscribers-clang-tools-extra Author: Balazs Benics (steakhal) ChangesFinds redundant lookups to set|map containers. I've implemented this check in the Clang Static Analyzer, but it didn't really bring much value compared to its cost. My experience of this checker so far on the 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. Let me know what you think. Patch is 35.63 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/125420.diff 13 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/performance/CMakeLists.txt b/clang-tools-extra/clang-tidy/performance/CMakeLists.txt
index c6e547c5089fb0..6f907852c9fd1c 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 9e0fa6f88b36a0..a613114e6b83bd 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 00000000000000..fefb28682c5b8a
--- /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 00000000000000..b37e1c2234c767
--- /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 3bddeeda06e06b..2287e9f5caaf95 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 7b9b905ef76715..43f3d147c4c309 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 00000000000000..6f25215e867ead
--- /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/Inputs/Headers/functional b/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/functional
new file mode 100644
index 00000000000000..be845b3c7e68a7
--- /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 00000000000000..1c7b26c93f7270
--- /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...
[truncated]
|
clang-tools-extra/clang-tidy/performance/RedundantLookupCheck.cpp
Outdated
Show resolved
Hide resolved
|
|
||
| The regular expression matching the type of the container objects. | ||
| This is matched in a case insensitive manner. | ||
| Default is ``set|map``. |
There was a problem hiding this comment.
| Default is ``set|map``. | |
| Default is `set|map`. |
Please use single back-ticks for option names and values.
How about unordered_map/unordered_set?
There was a problem hiding this comment.
How about unordered_map/unordered_set?
They would match the set|map case-insensitive regex.
There was a problem hiding this comment.
That would also match {flat_|unordered_}{multi}{set|map} from the standard which appear to have similar interfaces which should be fine. But maybe it should be limited to the std namespace so it does not apply to any implementation.
And it would also match an object like class my_mmap.
There was a problem hiding this comment.
I intentionally left it matching on anything that has map or set spelled. This is to also match DenseMap DenseSet of llvm for instance.
There was a problem hiding this comment.
Maybe at least make it set$|map$ so it only matches at the end of the string instead of random occurrences.
There was a problem hiding this comment.
In that case it wouldnt match llvm::SetVector.
There was a problem hiding this comment.
But it would match class UseTest or class Settings.
In that case it wouldnt match
llvm::SetVector.
Maybe such classes should be explicitly be specified.
I am not a fan of including classes which a regular users does not come across i.e. llvm::. Even including ``boost::` specific stuff feels like a stretch to me. But let's not get too off-topic and wait for other people to chime in.
Making it so broad might even affect the performance but let's wait for the numbers first. I will build it locally an give it a spin with the next few days. (hopefully)
There was a problem hiding this comment.
I share your concern. However, there is more to consider here.
I think, this check should work with user-defined containers out of the box, to have a better off the shelf experience.
If they find FPs, they can override this regex, and that's it.
I think the chances as slim to have container-like APIs like I'm matching here. For example, exatcly single argument "insert".
I could restrict the matches by requiring more things. For example, key_type type alias defined in the container, or something like that.
However, for doing that, I'd first ask if this misclassification really happens in practice to justify the complexity.
My experience with the FPs is that there are sharp edges with the check, but this one is not one of those.
clang-tools-extra/docs/clang-tidy/checks/performance/redundant-lookup.rst
Outdated
Show resolved
Hide resolved
clang-tools-extra/docs/clang-tidy/checks/performance/redundant-lookup.rst
Outdated
Show resolved
Hide resolved
Co-authored-by: EugeneZelenko <[email protected]>
Co-authored-by: EugeneZelenko <[email protected]>
|
Could you please run this with |
I plan to re-run it on clang soon, and share the results. |
|
|
||
| void RedundantLookupCheck::onEndOfTranslationUnit() { | ||
| auto ByBeginLoc = [this](const CallExpr *Lookup1, const CallExpr *Lookup2) { | ||
| return SM->isBeforeInTranslationUnit(Lookup1->getBeginLoc(), |
There was a problem hiding this comment.
Better would be to use ExprSequence like bugprone-use-after-move check does.
There was a problem hiding this comment.
I didn't know of this ExprSequence utility. I had a quick look at this and it looks really nice.
In my case, I use this predicate to sort the findings, to always pick a deterministic error location, and to construct meaningful notes in order. That said, this is not used for driving a matching decision.
In the checker, I intentionally let the design open for matching cases like this:
if (coin()) {
map[key] = foo();
} else {
map[key] = bar();
}Because similar patterns could be frequently refactored into: map[key] = coin() ? foo() : bar();
While requiring a sequence relationship wouldn't allow matching this.
But its a good point that likely such a case would warrant a specific diagnostic message for delivering that it's more likely a FP.
However, I admit that it probably needs some refinement, because if the lookup group spans across many many lines, then the report becomes less actionable. Not to mention if some loop between some lookups actually modify the map.
I think I'd need more field experience to make a justified choice here.
|
@steakhal I just tried this PR on the LLVM code base (that is, |
clang-tools-extra/docs/clang-tidy/checks/performance/redundant-lookup.rst
Show resolved
Hide resolved
I've picked a heavy TU of clang for the test: The check if (Param->hasUnparsedDefaultArg()) {
assert(!RewrittenInit && "Should not have a rewritten init expression yet");
// If we've already cleared out the location for the default argument,
// that means we're parsing it right now.
if (!UnparsedDefaultArgLocs.count(Param)) { // <---------------------- first lookup
Diag(Param->getBeginLoc(), diag::err_recursive_default_argument) << FD;
Diag(CallLoc, diag::note_recursive_default_argument_used_here);
Param->setInvalidDecl();
return true;
}
Diag(CallLoc, diag::err_use_of_default_argument_to_function_declared_later)
<< FD << cast<CXXRecordDecl>(FD->getDeclContext());
Diag(UnparsedDefaultArgLocs[Param], // <------------------------- second lookup
diag::note_default_argument_declared_here);
return true;
} |
| - New :doc:`performance-redundant-lookup | ||
| <clang-tidy/checks/performance/redundant-lookup>` check. | ||
|
|
||
| This check warns about potential redundant container lookup operations within |
There was a problem hiding this comment.
This check should be omitted.
There was a problem hiding this comment.
Dropped in c6a681c
|
Performance looks good. Everything under 5% is acceptable. The top three entries should probably get tickets so they get looked into. The amount seems too much for what it actually provides. |
Yea, my idea was to give a bit of context of how it compares to the resr of the checks. Good to know its acceptable! Thanks. |
|
I ran it on the Cppcheck codebase and there were several cases I would consider false positives as well as some which are really awkward to fix (in some cases leading to potentially unnecessary lookups). I will provide some examples in the next few days. |
I ran it on cppcheck, and I had 88 reports in total. Note that sometimes the looked ugly, and had a lot of nesting. It may be possible to refactor but I'd decide not touching it. This is one form of a FP. After seeing the results on cppcheck, I think it would make sense to use if (cond)
map[key] = foo();
else
map[key] = bar();I've seen code like this part of the TP set. BTW I've not seen any misclassification of "map" or "set" objects due to the relaxed regex. |
IMO this is fine, I don't think there should be a warning. In this simple case it's equally readable to take the value reference before each branch. In more complicated cases it might actually be more readable to have this pattern versus keeping one (or more) references to values in a map. It also makes reference invalidation issues harder to catch. |
|
I revisited the check using My approach with my prototype using
This got fairly involved, and likely scales really poorly with the number of lookups using the same key and container. Here are a couple of examples: So, if |
|
@steakhal, I don't meant to delay the landing of this PR, but I just want to bring up the following. I'm wondering if your checker can catch repeated lookups within a loop like so: in a function that does not reference |
|
This AST-based implementation no. There the loop would be unrolled about 3-4 times, so the checker should get to see the redundant lookups. I'll check if my assessment is correct. But I decided that the complexity does not worth the CSA based solution compared to the AST based. Both has their ups and downs. |
|
I think we should agree on an initial implementation to be landed and open separate tickets and not a single meta one for additional patterns to detect. |
|
Did you delete this by accident? 😕 Or did I say something wrong? 😟 |
No, not at all. I had difficulties with GitHub that I wanted to solve, and as a last measure I deleted my fork. I also want to get a minimal viable implementation merged, because I didn't mean to fine-tune this forever. I'll recreate this PR once I made some improvements. I'd expect it in a week or so. Sorry for the noise of getting this PR accidentally (and permanently) closed :( |
|
Thanks for the insight. Take your time. |
So, I spent a few hours experimenting and it's really difficult to do, only using It got fairly complicated and still buggy. I definitely think it's achievable, it's just too much that I could invest into. So, let's say, I'd stick with the current version of this branch, what would be left to continue the review? @firewave |
|
@steakhal: Please fix documentation and Release Notes wording that I mentioned already. |
Thanks, fixed! |
Finds redundant lookups to set|map containers.
For example:
map.count(key) && map[key] < thresholdI'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 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
clangcode 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: 0e62c74, 9333d8f, d0a142e, 0d21ef4, 65708ba, 16d4453.
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.