[clang-tidy] Add new performance-expensive-flat-container-operation check

clang-tools-extra/clang-tidy/performance/CMakeLists.txt

@@ -6,6 +6,7 @@ set(LLVM_LINK_COMPONENTS
 add_clang_library(clangTidyPerformanceModule STATIC
   AvoidEndlCheck.cpp
   EnumSizeCheck.cpp
+  ExpensiveFlatContainerOperationCheck.cpp
   FasterStringFindCheck.cpp
   ForRangeCopyCheck.cpp
   ImplicitConversionInLoopCheck.cpp

clang-tools-extra/clang-tidy/performance/ExpensiveFlatContainerOperationCheck.cpp

@@ -0,0 +1,104 @@
+//===--- ExpensiveFlatContainerOperationCheck.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 "ExpensiveFlatContainerOperationCheck.h"
+
+#include "../utils/OptionsUtils.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::performance {
+
+namespace {
+// TODO: folly::heap_vector_map?
+const auto DefaultFlatContainers =
+    "::std::flat_map; ::std::flat_multimap;"
+    "::std::flat_set; ::std::flat_multiset;"
+    "::boost::container::flat_map; ::boost::container::flat_multimap;"
+    "::boost::container::flat_set; ::boost::container::flat_multiset;"
+    "::folly::sorted_vector_map; ::folly::sorted_vector_set;";
+} // namespace
+
+ExpensiveFlatContainerOperationCheck::ExpensiveFlatContainerOperationCheck(
+    StringRef Name, ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      WarnOutsideLoops(Options.get("WarnOutsideLoops", false)),
+      FlatContainers(utils::options::parseStringList(
+          Options.get("FlatContainers", DefaultFlatContainers))) {}
+
+void ExpensiveFlatContainerOperationCheck::registerMatchers(
+    MatchFinder *Finder) {
+  const auto OnSoughtFlatContainer =
+      callee(cxxMethodDecl(ofClass(cxxRecordDecl(hasAnyName(FlatContainers)))));
+
+  // Any emplace-style or insert_or_assign call is a single-element operation.
+  const auto HasEmplaceOrInsertorAssignCall = callee(cxxMethodDecl(hasAnyName(
+      "emplace", "emplace_hint", "try_emplace", "insert_or_assign")));
+
+  // Erase calls with a single argument are single-element operations.
+  const auto HasEraseCallWithOneArgument = cxxMemberCallExpr(
+      argumentCountIs(1), callee(cxxMethodDecl(hasName("erase"))));
+
+  // TODO: insert.
+
+  const auto SoughtFlatContainerOperation =
+      cxxMemberCallExpr(
+          OnSoughtFlatContainer,
+          anyOf(HasEmplaceOrInsertorAssignCall, HasEraseCallWithOneArgument))
+          .bind("call");
+
+  if (WarnOutsideLoops) {
+    Finder->addMatcher(SoughtFlatContainerOperation, this);
+    return;
+  }
+
+  Finder->addMatcher(
+      mapAnyOf(whileStmt, forStmt, cxxForRangeStmt, doStmt)
+          .with(stmt(
+              stmt().bind("loop"),
+              forEachDescendant(cxxMemberCallExpr(
+                  SoughtFlatContainerOperation,
+                  // Common false positive: variable is declared directly within
+                  // the loop. Note that this won't catch cases where the
+                  // container is a member of a class declared in the loop.
+                  // More robust lifetime analysis would be required to catch
+                  // those cases, but this should filter out the most common
+                  // false positives.
+                  unless(onImplicitObjectArgument(declRefExpr(hasDeclaration(
+                      decl(hasAncestor(stmt(equalsBoundNode("loop")))))))))))),
+      this);
+}
+
+void ExpensiveFlatContainerOperationCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  const auto *Call = Result.Nodes.getNodeAs<CXXMemberCallExpr>("call");
+
+  diag(Call->getExprLoc(),
+       "Single element operations are expensive for flat containers. "
+       "Consider using available bulk operations instead, aggregating values "
+       "beforehand if needed.");
+}
+
+void ExpensiveFlatContainerOperationCheck::storeOptions(
+    ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "WarnOutsideLoops", WarnOutsideLoops);
+  Options.store(Opts, "FlatContainers",
+                utils::options::serializeStringList(FlatContainers));
+}
+
+bool ExpensiveFlatContainerOperationCheck::isLanguageVersionSupported(
+    const LangOptions &LangOpts) const {
+  return LangOpts.CPlusPlus;
+}
+
+std::optional<TraversalKind>
+ExpensiveFlatContainerOperationCheck::getCheckTraversalKind() const {
+  return TK_IgnoreUnlessSpelledInSource;
+}
+} // namespace clang::tidy::performance

clang-tools-extra/clang-tidy/performance/ExpensiveFlatContainerOperationCheck.h

@@ -0,0 +1,37 @@
+//===--- ExpensiveFlatContainerOperationCheck.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_EXPENSIVEFLATCONTAINEROPERATIONCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_EXPENSIVEFLATCONTAINEROPERATIONCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang::tidy::performance {
+
+/// Warns when calling an O(N) operation on a flat container.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/performance/expensive-flat-container-operation.html
+class ExpensiveFlatContainerOperationCheck : public ClangTidyCheck {
+public:
+  ExpensiveFlatContainerOperationCheck(StringRef Name,
+                                       ClangTidyContext *Context);
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override;
+  std::optional<TraversalKind> getCheckTraversalKind() const override;
+
+private:
+  bool WarnOutsideLoops;
+  std::vector<StringRef> FlatContainers;
+};
+
+} // namespace clang::tidy::performance
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_EXPENSIVEFLATCONTAINEROPERATIONCHECK_H

clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp

@@ -11,6 +11,7 @@
 #include "../ClangTidyModuleRegistry.h"
 #include "AvoidEndlCheck.h"
 #include "EnumSizeCheck.h"
+#include "ExpensiveFlatContainerOperationCheck.h"
 #include "FasterStringFindCheck.h"
 #include "ForRangeCopyCheck.h"
 #include "ImplicitConversionInLoopCheck.h"
@@ -37,6 +38,8 @@ class PerformanceModule : public ClangTidyModule {
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<AvoidEndlCheck>("performance-avoid-endl");
     CheckFactories.registerCheck<EnumSizeCheck>("performance-enum-size");
+    CheckFactories.registerCheck<ExpensiveFlatContainerOperationCheck>(
+        "performance-expensive-flat-container-operation");
     CheckFactories.registerCheck<FasterStringFindCheck>(
         "performance-faster-string-find");
     CheckFactories.registerCheck<ForRangeCopyCheck>(

clang-tools-extra/docs/ReleaseNotes.rst

@@ -121,6 +121,11 @@ New checks
   Gives warnings for tagged unions, where the number of tags is
   different from the number of data members inside the union.
 
+- New :doc:`performance-expensive-flat-container-operation
+  <clang-tidy/checks/performance/expensive-flat-container-operation>` check.
+
+  Warns when calling an O(N) operation on a flat container.
+
 - New :doc:`portability-template-virtual-member-function
   <clang-tidy/checks/portability/template-virtual-member-function>` check.
 

clang-tools-extra/docs/clang-tidy/checks/list.rst

@@ -328,6 +328,7 @@ Clang-Tidy Checks
    :doc:`openmp-use-default-none <openmp/use-default-none>`,
    :doc:`performance-avoid-endl <performance/avoid-endl>`, "Yes"
    :doc:`performance-enum-size <performance/enum-size>`,
+   :doc:`performance-expensive-flat-container-operation <performance/expensive-flat-container-operation>`,
    :doc:`performance-faster-string-find <performance/faster-string-find>`, "Yes"
    :doc:`performance-for-range-copy <performance/for-range-copy>`, "Yes"
    :doc:`performance-implicit-conversion-in-loop <performance/implicit-conversion-in-loop>`,

clang-tools-extra/docs/clang-tidy/checks/performance/expensive-flat-container-operation.rst

@@ -0,0 +1,82 @@
+.. title:: clang-tidy - performance-expensive-flat-container-operation
+
+performance-expensive-flat-container-operation
+==============================================
+
+Warns when calling an O(N) operation on a flat container.
+
+This check operates on vector-based flat containers such as
+``std::flat_(map|set)``, ``boost::container::flat_(map|set)``, or
+``folly::sorted_vector_(map|set)``. While these containers' behavior is
+identical to usual maps/sets, the insert and erase operations are O(N). This
+check flags such operations, which are a common bad pattern, notably in loops.
+
+Below is an example of a typical bad pattern: inserting some values one by one
+into a flat container. This is O(N^2), as the container will need to shift
+elements right after each insertion.
+
+.. code-block:: c++
+
+    std::random_device generator;
+    std::uniform_int_distribution<int> distribution;
+
+    std::flat_set<int> set;
+    for (auto i = 0; i < N; ++i) {
+        set.insert(distribution(generator));
+    }
+
+The code above can be improved using a temporary vector, later inserting all
+values at once into the ``flat_set``.
+
+.. code-block:: c++
+
+    std::vector<int> temporary;
+    for (auto i = 0; i < N; ++i) {
+        temporary.push_back(distribution(generator));
+    }
+    std::flat_set<int> set(temporary.begin(), temporary.end());
+
+    // Or even better when possible, moving the temporary container:
+    // std::flat_set<int> set(std::move(temporary));
+
+For expensive-to-copy objects, ``std::move_iterator`` should be used.
+When possible, the temporary container can be moved directly into the flat
+container. When it is known that the inserted keys are sorted and uniqued, such
+as cases when they come from another flat container, ``std::sorted_unique`` can
+be used when inserting to save more cycles. Finally, if order is not important,
+hash-based containers can provide better performance.
+
+Limitations
+-----------
+
+This check is not capable of flagging insertions into a map via ``operator[]``,
+as it is not possible at compile-time to know whether it will trigger an
+insertion or a simple lookup. These cases have to be detected using dynamic
+profiling.
+
+This check is also of course not able to detect single element operations in
+loops crossing function boundaries. A more robust static analysis would be
+necessary to detect these cases.
+
+Options
+-------
+
+.. option:: WarnOutsideLoops
+
+    When disabled, the check will only warn when the single element operation is
+    directly enclosed by a loop, hence directly actionable. At the very least,
+    these cases can be improved using some temporary container.
+
+    When enabled, all insert and erase operations will be flagged.
+
+    Default is `false`.
+
+.. option:: FlatContainers
+
+    A semicolon-separated list of flat containers, with ``insert``, ``emplace``
+    and/or ``erase`` operations.
+
+    Default includes ``std::flat_(map|set)``, ``flat_multi(map|set)``,
+    ``boost::container::flat_(map|set)``,
+    ``boost::container::flat_multi(map|set)``, and
+    ``folly::sorted_vector_(map|set)``.