[clang-tools-extra] [clang-tidy] Add performance-redundant-lookup check (PR #125420)
steakhal wrote: > @steakhal: Please fix documentation and Release Notes wording that I > mentioned already. Thanks, fixed! I'm actually more curious about the semantic requirements, and about what directions should I explore more. https://github.com/llvm/llvm-project/pull/125420 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add performance-redundant-lookup check (PR #125420)
@@ -91,6 +91,12 @@ Improvements to clang-tidy New checks ^^ +- New :doc:`performance-redundant-lookup + ` check. + + This check warns about potential redundant container lookup operations within steakhal wrote: Dropped in c6a681c955fef8b933374a05a157f48ba4ead360 https://github.com/llvm/llvm-project/pull/125420 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add performance-redundant-lookup check (PR #125420)
EugeneZelenko wrote: @steakhal: Please fix documentation and Release Notes wording that I mentioned already. https://github.com/llvm/llvm-project/pull/125420 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add performance-redundant-lookup check (PR #125420)
steakhal wrote: > Thanks for the insight. Take your time. So, I spent a few hours experimenting and it's really difficult to do, only using `ExprSequence`. Just to recap, my idea was to match the assignments and inc/dec unary operator calls to detect if the container or the key object is mutated. Then partition the lookup events if any of the lookups were "separated" by such a mutation. Then check if the remaining group has at least 2 lookups, if so, report them. 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](https://github.com/steakhal/llvm-project/tree/bb/tidy-add-redundant-lookup-check), what would be left to continue the review? @firewave https://github.com/llvm/llvm-project/pull/125420 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add performance-redundant-lookup check (PR #125420)
https://github.com/steakhal closed https://github.com/llvm/llvm-project/pull/125420 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add performance-redundant-lookup check (PR #125420)
firewave wrote: 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. https://github.com/llvm/llvm-project/pull/125420 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add performance-redundant-lookup check (PR #125420)
steakhal wrote: I think this case would be caught by the static analyzer based implementation. 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. https://github.com/llvm/llvm-project/pull/125420 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add performance-redundant-lookup check (PR #125420)
kazutakahirata wrote: @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: ``` for (int A : Vec) Map[Key].push_back(A); ``` in a function that does not reference `Map` anywhere else. That is, statically, there is only one statement doing map lookups, but dynamically, the same lookup is repeated over and over. Thanks! https://github.com/llvm/llvm-project/pull/125420 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add performance-redundant-lookup check (PR #125420)
steakhal wrote: I revisited the check using `ExprSequence`. It became more actionable from what I can tell, but exposes weaknesses of `ExprSequence` too, which I'll later demonstrate. My approach with my prototype using `ExprSequence` was: 1) Collect lookups just like before. 2) Take the cross product of the lookups, and build a graph using the `inSequence` relationship among the lookups. 3) Take the nodes that has no parents, aka. roots of the `inSequence` graph, because I'll report a separate "bug" for the reachable lookups from each Root. 4) Pick the last lookup of the group as the diagnostic location, and add notes for the rest. This got fairly involved, and likely scales really poorly with the number of lookups using the same key and container. The results are mixed, and I think it's caused by `ExprSequence`, and hallucinates `inSequence` relationship for Stmt pairs where it actually shouldn't. Here are a couple of examples:    So, if `ExprSequence` would honor `return` statements and if blocks, then it would be a viable direction to move forward. https://github.com/llvm/llvm-project/pull/125420 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add performance-redundant-lookup check (PR #125420)
steakhal wrote: > 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. See the results at [this](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?review-status=Confirmed%20bug&review-status=False%20positive&detection-status=New&detection-status=Reopened&detection-status=Unresolved&run=steakhal-redundant-lookups-in-cppcheck&is-unique=off&diff-type=New) CodeChecker instance. Apply filters to look for only TPs or FPs. [Thanks for Ericsson hosting the instance btw] I looked each of the reports and classified them as "true positive" if I (likely) could refactor the code and would make sense doing so. FP otherwise if I wouldn't refactor the code. This gave me 56 TPs and 32 FPs. 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. The FP TP classification is of course arbitrary, and you may disagree with what I choose. These TPs are the cases where I think the code could be and should be refactored, nothing more. After seeing the results on cppcheck, I think it would make sense to use `ExprSequence` to ensure that the lookups are not on mutually exclusive paths for instance. It would also match better with the category of the check "performance". On the down side, it would make us no longer diagnose code like this: ```c++ 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. https://github.com/llvm/llvm-project/pull/125420 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add performance-redundant-lookup check (PR #125420)
firewave wrote: 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. https://github.com/llvm/llvm-project/pull/125420 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add performance-redundant-lookup check (PR #125420)
steakhal wrote: > 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. https://github.com/llvm/llvm-project/pull/125420 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add performance-redundant-lookup check (PR #125420)
firewave wrote: 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. https://github.com/llvm/llvm-project/pull/125420 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add performance-redundant-lookup check (PR #125420)
@@ -91,6 +91,12 @@ Improvements to clang-tidy New checks ^^ +- New :doc:`performance-redundant-lookup + ` check. + + This check warns about potential redundant container lookup operations within EugeneZelenko wrote: `This check` should be omitted. https://github.com/llvm/llvm-project/pull/125420 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add performance-redundant-lookup check (PR #125420)
@@ -0,0 +1,147 @@ +.. title:: clang-tidy - performance-redundant-lookup + +performance-redundant-lookup + + +This check warns about potential redundant container lookup operations within steakhal wrote: Fixed in d9d3984a056192b8ef61f385090d90c55774123d https://github.com/llvm/llvm-project/pull/125420 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add performance-redundant-lookup check (PR #125420)
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
Date: Sun, 2 Feb 2025 17:35:39 +0100
Subject: [PATCH 1/8] [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 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(
"performance-noexcept-swap");
+CheckFactories.registerCheck(
+"performance-redundant-lookup");
CheckFactories.registerCheck(
"performance-trivially-destructible");
CheckFactories.registerCheck(
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 00..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(ClangTidyOption
[clang-tools-extra] [clang-tidy] Add performance-redundant-lookup check (PR #125420)
steakhal wrote:
> > Could you please run this with `--enable-check-profile` to see how heavy it
> > is?
>
> I plan to re-run it on clang soon, and share the results.
I've picked a heavy TU of clang for the test: `clang/lib/Sema/SemaExpr.cpp`
```
===-===
clang-tidy checks profiling
===-===
Total Execution Time: 12.6394 seconds (12.6125 wall clock)
---User Time--- --System Time-- --User+System-- ---Wall Time--- ---
Name ---
1.3723 ( 19.8%) 1.3141 ( 23.0%) 2.6865 ( 21.3%) 2.6692 ( 21.2%)
misc-unused-using-decls
0.7283 ( 10.5%) 0.6586 ( 11.5%) 1.3869 ( 11.0%) 1.3875 ( 11.0%)
llvm-prefer-isa-or-dyn-cast-in-conditionals
0.9165 ( 13.2%) 0.2823 ( 4.9%) 1.1987 ( 9.5%) 1.1982 ( 9.5%)
llvm-qualified-auto
0.5424 ( 7.8%) 0.4717 ( 8.3%) 1.0141 ( 8.0%) 1.0058 ( 8.0%)
misc-unconventional-assign-operator
0.4127 ( 6.0%) 0.3865 ( 6.8%) 0.7991 ( 6.3%) 0.7982 ( 6.3%)
misc-misleading-identifier
0.4212 ( 6.1%) 0.3571 ( 6.2%) 0.7783 ( 6.2%) 0.7810 ( 6.2%)
misc-confusable-identifiers
0.4033 ( 5.8%) 0.3750 ( 6.6%) 0.7784 ( 6.2%) 0.7775 ( 6.2%)
misc-definitions-in-headers
0.3977 ( 5.7%) 0.3700 ( 6.5%) 0.7677 ( 6.1%) 0.7658 ( 6.1%)
misc-non-copyable-objects
0.2984 ( 4.3%) 0.2572 ( 4.5%) 0.5556 ( 4.4%) 0.5532 ( 4.4%)
misc-redundant-expression
0.2539 ( 3.7%) 0.2164 ( 3.8%) 0.4703 ( 3.7%) 0.4691 ( 3.7%)
performance-redundant-lookup
0.2140 ( 3.1%) 0.1966 ( 3.4%) 0.4107 ( 3.2%) 0.4092 ( 3.2%)
misc-misplaced-const
0.2021 ( 2.9%) 0.1826 ( 3.2%) 0.3846 ( 3.0%) 0.3911 ( 3.1%)
misc-use-internal-linkage
0.1293 ( 1.9%) 0.1195 ( 2.1%) 0.2488 ( 2.0%) 0.2495 ( 2.0%)
misc-new-delete-overloads
0.1141 ( 1.6%) 0.1046 ( 1.8%) 0.2187 ( 1.7%) 0.2189 ( 1.7%)
llvm-prefer-register-over-unsigned
0.1209 ( 1.7%) 0.0911 ( 1.6%) 0.2120 ( 1.7%) 0.2112 ( 1.7%)
misc-static-assert
0.1081 ( 1.6%) 0.0989 ( 1.7%) 0.2070 ( 1.6%) 0.2093 ( 1.7%)
llvm-twine-local
0.0673 ( 1.0%) 0.0670 ( 1.2%) 0.1343 ( 1.1%) 0.1325 ( 1.1%)
misc-unused-alias-decls
0.0693 ( 1.0%) 0.0633 ( 1.1%) 0.1326 ( 1.0%) 0.1309 ( 1.0%)
misc-uniqueptr-reset-release
0.0699 ( 1.0%) 0.0567 ( 1.0%) 0.1266 ( 1.0%) 0.1261 ( 1.0%)
llvm-else-after-return
0.0736 ( 1.1%) 0.0358 ( 0.6%) 0.1094 ( 0.9%) 0.1093 ( 0.9%)
llvm-namespace-comment
0.0100 ( 0.1%) 0.0091 ( 0.2%) 0.0191 ( 0.2%) 0.0190 ( 0.2%)
misc-misleading-bidirectional
0. ( 0.0%) 0. ( 0.0%) 0. ( 0.0%) 0. ( 0.0%)
misc-throw-by-value-catch-by-reference
6.9252 (100.0%) 5.7142 (100.0%) 12.6394 (100.0%) 12.6125 (100.0%) Total
```
The check `performance-redundant-lookup` had a single hit at line 5353 for that
TU, which was a TP:
```c++
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(FD->getDeclContext());
Diag(UnparsedDefaultArgLocs[Param], // <- second
lookup
diag::note_default_argument_declared_here);
return true;
}
```
https://github.com/llvm/llvm-project/pull/125420
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add performance-redundant-lookup check (PR #125420)
@@ -0,0 +1,147 @@ +.. title:: clang-tidy - performance-redundant-lookup + +performance-redundant-lookup + + +This check warns about potential redundant container lookup operations within EugeneZelenko wrote: Please synchronize with statement in Release Notes. https://github.com/llvm/llvm-project/pull/125420 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add performance-redundant-lookup check (PR #125420)
@@ -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(EnclosingFnKey);
+ const auto *LookupCall = Result.Nodes.getNodeAs(LookupCallKey);
+ const auto *Key = Result.Nodes.getNodeAs(LookupKey);
+ const auto *ContainerObject = Result.Nodes.getNodeAs(ObjKey);
[clang-tools-extra] [clang-tidy] Add performance-redundant-lookup check (PR #125420)
kazutakahirata wrote: @steakhal I just tried this PR on the LLVM code base (that is, `llvm-project/llvm`). I'm impressed how many real issues it finds. Thank you so much! https://github.com/llvm/llvm-project/pull/125420 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add performance-redundant-lookup check (PR #125420)
@@ -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(EnclosingFnKey);
+ const auto *LookupCall = Result.Nodes.getNodeAs(LookupCallKey);
+ const auto *Key = Result.Nodes.getNodeAs(LookupKey);
+ const auto *ContainerObject = Result.Nodes.getNodeAs(ObjKey);
[clang-tools-extra] [clang-tidy] Add performance-redundant-lookup check (PR #125420)
@@ -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".
+
+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``.
steakhal wrote:
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.
https://github.com/llvm/llvm-project/pull/125420
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add performance-redundant-lookup check (PR #125420)
@@ -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".
+
+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``.
firewave wrote:
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)
https://github.com/llvm/llvm-project/pull/125420
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add performance-redundant-lookup check (PR #125420)
@@ -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".
+
+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``.
steakhal wrote:
I intentionally left it matching on anything that has `map` or `set` spelled.
This is to also match `DenseMap` `DenseSet` of `llvm` for instance.
https://github.com/llvm/llvm-project/pull/125420
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add performance-redundant-lookup check (PR #125420)
@@ -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".
+
+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``.
steakhal wrote:
In that case it wouldnt match `llvm::SetVector`.
https://github.com/llvm/llvm-project/pull/125420
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add performance-redundant-lookup check (PR #125420)
@@ -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".
+
+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``.
firewave wrote:
Maybe at least make it `set$|map$` so it only matches at the end of the string
instead of random occurrences.
https://github.com/llvm/llvm-project/pull/125420
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add performance-redundant-lookup check (PR #125420)
steakhal wrote: > Could you please run this with `--enable-check-profile` to see how heavy it > is? I plan to re-run it on clang soon, and share the results. https://github.com/llvm/llvm-project/pull/125420 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add performance-redundant-lookup check (PR #125420)
firewave wrote: Could you please run this with `--enable-check-profile` to see how heavy it is? https://github.com/llvm/llvm-project/pull/125420 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add performance-redundant-lookup check (PR #125420)
@@ -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".
+
+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``.
EugeneZelenko wrote:
How about `boost`?
https://github.com/llvm/llvm-project/pull/125420
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add performance-redundant-lookup check (PR #125420)
@@ -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".
+
+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``.
firewave wrote:
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`.
https://github.com/llvm/llvm-project/pull/125420
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add performance-redundant-lookup check (PR #125420)
@@ -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(EnclosingFnKey);
+ const auto *LookupCall = Result.Nodes.getNodeAs(LookupCallKey);
+ const auto *Key = Result.Nodes.getNodeAs(LookupKey);
+ const auto *ContainerObject = Result.Nodes.getNodeAs(ObjKey);
[clang-tools-extra] [clang-tidy] Add performance-redundant-lookup check (PR #125420)
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
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(
"performance-noexcept-swap");
+CheckFactories.registerCheck(
+"performance-redundant-lookup");
CheckFactories.registerCheck(
"performance-trivially-destructible");
CheckFactories.registerCheck(
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 000..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(ClangTidy
[clang-tools-extra] [clang-tidy] Add performance-redundant-lookup check (PR #125420)
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
Date: Sun, 2 Feb 2025 17:35:39 +0100
Subject: [PATCH 1/5] [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(
"performance-noexcept-swap");
+CheckFactories.registerCheck(
+"performance-redundant-lookup");
CheckFactories.registerCheck(
"performance-trivially-destructible");
CheckFactories.registerCheck(
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 000..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(ClangTidy
[clang-tools-extra] [clang-tidy] Add performance-redundant-lookup check (PR #125420)
@@ -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".
+
+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``.
steakhal wrote:
> How about unordered_map/unordered_set?
They would match the `set|map` case-insensitive regex.
https://github.com/llvm/llvm-project/pull/125420
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add performance-redundant-lookup check (PR #125420)
@@ -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".
+
+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
+---
+
+ - 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.
EugeneZelenko wrote:
```suggestion
`LookupMethodNames` option.
```
https://github.com/llvm/llvm-project/pull/125420
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add performance-redundant-lookup check (PR #125420)
@@ -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".
+
+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``.
EugeneZelenko wrote:
```suggestion
Default is `set|map`.
```
Please use single back-ticks for option names and values.
How about `unordered_map/unordered_set`?
https://github.com/llvm/llvm-project/pull/125420
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add performance-redundant-lookup check (PR #125420)
@@ -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".
+
+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``.
EugeneZelenko wrote:
```suggestion
Default is `at;contains;count;find_as;find`.
```
https://github.com/llvm/llvm-project/pull/125420
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add performance-redundant-lookup check (PR #125420)
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
Date: Sun, 2 Feb 2025 17:35:39 +0100
Subject: [PATCH 1/4] [clang-tidy] Add performance-redundant-lookup check
Finds redundant lookups to set|map containers.
For example: `map.count(key) && map[key] < threshold`
---
.../clang-tidy/performance/CMakeLists.txt | 1 +
.../performance/PerformanceTidyModule.cpp | 3 +
.../performance/RedundantLookupCheck.cpp | 159 ++
.../performance/RedundantLookupCheck.h| 46 +
clang-tools-extra/docs/ReleaseNotes.rst | 5 +
.../docs/clang-tidy/checks/list.rst | 1 +
.../checks/performance/redundant-lookup.rst | 147
.../checkers/performance/redundant-lookup.cpp | 142
8 files changed, 504 insertions(+)
create mode 100644
clang-tools-extra/clang-tidy/performance/RedundantLookupCheck.cpp
create mode 100644
clang-tools-extra/clang-tidy/performance/RedundantLookupCheck.h
create mode 100644
clang-tools-extra/docs/clang-tidy/checks/performance/redundant-lookup.rst
create mode 100644
clang-tools-extra/test/clang-tidy/checkers/performance/redundant-lookup.cpp
diff --git a/clang-tools-extra/clang-tidy/performance/CMakeLists.txt
b/clang-tools-extra/clang-tidy/performance/CMakeLists.txt
index c6e547c5089fb0e..6f907852c9fd1c4 100644
--- a/clang-tools-extra/clang-tidy/performance/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/performance/CMakeLists.txt
@@ -21,6 +21,7 @@ add_clang_library(clangTidyPerformanceModule STATIC
NoexceptMoveConstructorCheck.cpp
NoexceptSwapCheck.cpp
PerformanceTidyModule.cpp
+ RedundantLookupCheck.cpp
TriviallyDestructibleCheck.cpp
TypePromotionInMathFnCheck.cpp
UnnecessaryCopyInitialization.cpp
diff --git a/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
b/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
index 9e0fa6f88b36a00..a613114e6b83bd3 100644
--- a/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
@@ -24,6 +24,7 @@
#include "NoexceptDestructorCheck.h"
#include "NoexceptMoveConstructorCheck.h"
#include "NoexceptSwapCheck.h"
+#include "RedundantLookupCheck.h"
#include "TriviallyDestructibleCheck.h"
#include "TypePromotionInMathFnCheck.h"
#include "UnnecessaryCopyInitialization.h"
@@ -62,6 +63,8 @@ class PerformanceModule : public ClangTidyModule {
"performance-noexcept-move-constructor");
CheckFactories.registerCheck(
"performance-noexcept-swap");
+CheckFactories.registerCheck(
+"performance-redundant-lookup");
CheckFactories.registerCheck(
"performance-trivially-destructible");
CheckFactories.registerCheck(
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 000..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(ClangTidy
[clang-tools-extra] [clang-tidy] Add performance-redundant-lookup check (PR #125420)
llvmbot wrote:
@llvm/pr-subscribers-clang-tidy
Author: Balazs Benics (steakhal)
Changes
Finds redundant lookups to set|map containers.
For example: `map.count(key) && map[key] < threshold`
---
I've implemented this check in the Clang Static Analyzer, but it didn't really
bring much value compared to its cost.
Because of this, I decided to have a second look and implement this in
clang-tidy.
As in the
[comment](https://github.com/llvm/llvm-project/issues/123376#issuecomment-2628966580)
people showed interest, I decided to give a final polish to the check and
publish it to ask for feedback.
My experience of this checker so far on the `clang` code base was that it
didn't produce many false-positives, but those are hard to avoid due to what
patterns we are looking for.
Frequently the findings were actionable and useful, and already fixed a couple
issue that I found: 0e62c748d440a6d12d190e951c987efe309f40d6,
9333d8fb0749b1ae2b152a1003d2ecc00027b3d5,
d0a142eaea03661e8399f2c1733b93d21d55dfee,
0d21ef4e6c50c7d4d591adf7e6dbd6232e8a99c4,
65708bad579229cd7f62b8d0eaefda4bb20eb6d8,
16d4453f2f5d9554ce39507fda0f33ce9066007b.
I've considered adding some fixits, and that may be possible if we focus on
some really frequent patterns, but I decided not to invest there.
I've considered techniques to limit the scope of what "lookups" should be
bundled together, for example the lookups that are in close proximity, or
lookups that are within the same lexical scope, or there are no mutations
between the lookups; but ultimately I found any of these arbitrary and
confusing - though I admit, there could be improvements.
Let me know what you think.
---
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:
- (modified) clang-tools-extra/clang-tidy/performance/CMakeLists.txt (+1)
- (modified) clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
(+3)
- (added) clang-tools-extra/clang-tidy/performance/RedundantLookupCheck.cpp
(+159)
- (added) clang-tools-extra/clang-tidy/performance/RedundantLookupCheck.h (+46)
- (modified) clang-tools-extra/docs/ReleaseNotes.rst (+5)
- (modified) clang-tools-extra/docs/clang-tidy/checks/list.rst (+1)
- (added)
clang-tools-extra/docs/clang-tidy/checks/performance/redundant-lookup.rst
(+147)
- (added) clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/functional
(+21)
- (added) clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/map (+167)
- (added) clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/memory
(+10)
- (added) clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/set (+145)
- (added) clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/utility
(+13)
- (added)
clang-tools-extra/test/clang-tidy/checkers/performance/redundant-lookup.cpp
(+142)
``diff
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(
"performance-noexcept-swap");
+CheckFactories.registerCheck(
+"performance-redundant-lookup");
CheckFactories.registerCheck(
"performance-trivially-destructible");
CheckFactories.registerCheck(
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 000..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.
[clang-tools-extra] [clang-tidy] Add performance-redundant-lookup check (PR #125420)
llvmbot wrote:
@llvm/pr-subscribers-clang-tools-extra
Author: Balazs Benics (steakhal)
Changes
Finds redundant lookups to set|map containers.
For example: `map.count(key) && map[key] < threshold`
---
I've implemented this check in the Clang Static Analyzer, but it didn't really
bring much value compared to its cost.
Because of this, I decided to have a second look and implement this in
clang-tidy.
As in the
[comment](https://github.com/llvm/llvm-project/issues/123376#issuecomment-2628966580)
people showed interest, I decided to give a final polish to the check and
publish it to ask for feedback.
My experience of this checker so far on the `clang` code base was that it
didn't produce many false-positives, but those are hard to avoid due to what
patterns we are looking for.
Frequently the findings were actionable and useful, and already fixed a couple
issue that I found: 0e62c748d440a6d12d190e951c987efe309f40d6,
9333d8fb0749b1ae2b152a1003d2ecc00027b3d5,
d0a142eaea03661e8399f2c1733b93d21d55dfee,
0d21ef4e6c50c7d4d591adf7e6dbd6232e8a99c4,
65708bad579229cd7f62b8d0eaefda4bb20eb6d8,
16d4453f2f5d9554ce39507fda0f33ce9066007b.
I've considered adding some fixits, and that may be possible if we focus on
some really frequent patterns, but I decided not to invest there.
I've considered techniques to limit the scope of what "lookups" should be
bundled together, for example the lookups that are in close proximity, or
lookups that are within the same lexical scope, or there are no mutations
between the lookups; but ultimately I found any of these arbitrary and
confusing - though I admit, there could be improvements.
Let me know what you think.
---
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:
- (modified) clang-tools-extra/clang-tidy/performance/CMakeLists.txt (+1)
- (modified) clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
(+3)
- (added) clang-tools-extra/clang-tidy/performance/RedundantLookupCheck.cpp
(+159)
- (added) clang-tools-extra/clang-tidy/performance/RedundantLookupCheck.h (+46)
- (modified) clang-tools-extra/docs/ReleaseNotes.rst (+5)
- (modified) clang-tools-extra/docs/clang-tidy/checks/list.rst (+1)
- (added)
clang-tools-extra/docs/clang-tidy/checks/performance/redundant-lookup.rst
(+147)
- (added) clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/functional
(+21)
- (added) clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/map (+167)
- (added) clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/memory
(+10)
- (added) clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/set (+145)
- (added) clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/utility
(+13)
- (added)
clang-tools-extra/test/clang-tidy/checkers/performance/redundant-lookup.cpp
(+142)
``diff
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(
"performance-noexcept-swap");
+CheckFactories.registerCheck(
+"performance-redundant-lookup");
CheckFactories.registerCheck(
"performance-trivially-destructible");
CheckFactories.registerCheck(
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 00..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.
[clang-tools-extra] [clang-tidy] Add performance-redundant-lookup check (PR #125420)
https://github.com/steakhal created
https://github.com/llvm/llvm-project/pull/125420
Finds redundant lookups to set|map containers.
For example: `map.count(key) && map[key] < threshold`
---
I've implemented this check in the Clang Static Analyzer, but it didn't really
bring much value compared to its cost.
Because of this, I decided to have a second look and implement this in
clang-tidy.
As in the
[comment](https://github.com/llvm/llvm-project/issues/123376#issuecomment-2628966580)
people showed interest, I decided to give a final polish to the check and
publish it to ask for feedback.
My experience of this checker so far on the `clang` code base was that it
didn't produce many false-positives, but those are hard to avoid due to what
patterns we are looking for.
Frequently the findings were actionable and useful, and already fixed a couple
issue that I found: 0e62c748d440a6d12d190e951c987efe309f40d6,
9333d8fb0749b1ae2b152a1003d2ecc00027b3d5,
d0a142eaea03661e8399f2c1733b93d21d55dfee,
0d21ef4e6c50c7d4d591adf7e6dbd6232e8a99c4,
65708bad579229cd7f62b8d0eaefda4bb20eb6d8,
16d4453f2f5d9554ce39507fda0f33ce9066007b.
I've considered adding some fixits, and that may be possible if we focus on
some really frequent patterns, but I decided not to invest there.
I've considered techniques to limit the scope of what "lookups" should be
bundled together, for example the lookups that are in close proximity, or
lookups that are within the same lexical scope, or there are no mutations
between the lookups; but ultimately I found any of these arbitrary and
confusing - though I admit, there could be improvements.
Let me know what you think.
>From 78390b2af01fcd5acfbd2a313852962c873e5611 Mon Sep 17 00:00:00 2001
From: Balazs Benics
Date: Sun, 2 Feb 2025 17:35:39 +0100
Subject: [PATCH 1/2] [clang-tidy] Add performance-redundant-lookup check
Finds redundant lookups to set|map containers.
For example: `map.count(key) && map[key] < threshold`
---
.../clang-tidy/performance/CMakeLists.txt | 1 +
.../performance/PerformanceTidyModule.cpp | 3 +
.../performance/RedundantLookupCheck.cpp | 159 ++
.../performance/RedundantLookupCheck.h| 46 +
clang-tools-extra/docs/ReleaseNotes.rst | 5 +
.../docs/clang-tidy/checks/list.rst | 1 +
.../checks/performance/redundant-lookup.rst | 147
.../checkers/performance/redundant-lookup.cpp | 142
8 files changed, 504 insertions(+)
create mode 100644
clang-tools-extra/clang-tidy/performance/RedundantLookupCheck.cpp
create mode 100644
clang-tools-extra/clang-tidy/performance/RedundantLookupCheck.h
create mode 100644
clang-tools-extra/docs/clang-tidy/checks/performance/redundant-lookup.rst
create mode 100644
clang-tools-extra/test/clang-tidy/checkers/performance/redundant-lookup.cpp
diff --git a/clang-tools-extra/clang-tidy/performance/CMakeLists.txt
b/clang-tools-extra/clang-tidy/performance/CMakeLists.txt
index c6e547c5089fb0e..6f907852c9fd1c4 100644
--- a/clang-tools-extra/clang-tidy/performance/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/performance/CMakeLists.txt
@@ -21,6 +21,7 @@ add_clang_library(clangTidyPerformanceModule STATIC
NoexceptMoveConstructorCheck.cpp
NoexceptSwapCheck.cpp
PerformanceTidyModule.cpp
+ RedundantLookupCheck.cpp
TriviallyDestructibleCheck.cpp
TypePromotionInMathFnCheck.cpp
UnnecessaryCopyInitialization.cpp
diff --git a/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
b/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
index 9e0fa6f88b36a00..a613114e6b83bd3 100644
--- a/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
@@ -24,6 +24,7 @@
#include "NoexceptDestructorCheck.h"
#include "NoexceptMoveConstructorCheck.h"
#include "NoexceptSwapCheck.h"
+#include "RedundantLookupCheck.h"
#include "TriviallyDestructibleCheck.h"
#include "TypePromotionInMathFnCheck.h"
#include "UnnecessaryCopyInitialization.h"
@@ -62,6 +63,8 @@ class PerformanceModule : public ClangTidyModule {
"performance-noexcept-move-constructor");
CheckFactories.registerCheck(
"performance-noexcept-swap");
+CheckFactories.registerCheck(
+"performance-redundant-lookup");
CheckFactories.registerCheck(
"performance-trivially-destructible");
CheckFactories.registerCheck(
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 000..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/LICENS
