[PATCH] D150257: [clangd] Initialize clang-tidy modules only once

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang-tools-extra/clangd/ParsedAST.cpp:478
     trace::Span Tracer("ClangTidyInit");
-    tidy::ClangTidyCheckFactories CTFactories;
-    for (const auto &E : tidy::ClangTidyModuleRegistry::entries())
-      E.instantiate()->addCheckFactories(CTFactories);
+    static const tidy::ClangTidyCheckFactories CTFactories = [] {
+      tidy::ClangTidyCheckFactories CTFactories;
----------------
as written this creates a global destructor

instead, prefer heap-allocating and never freeing (the static variable should be a pointer or non-lifetime-extending reference)


================
Comment at: clang-tools-extra/clangd/TidyProvider.cpp:289
+  // time. So cache the results once.
+  static const auto Opts = [] {
+    auto Opts = tidy::ClangTidyOptions::getDefaults();
----------------
same here, avoid the destructor


================
Comment at: clang-tools-extra/clangd/TidyProvider.cpp:289
+  // time. So cache the results once.
+  static const auto Opts = [] {
+    auto Opts = tidy::ClangTidyOptions::getDefaults();
----------------
sammccall wrote:
> same here, avoid the destructor
nit: call this DefaultOpts, as it's in general not the opts we're calculating


================
Comment at: clang-tools-extra/clangd/TidyProvider.cpp:294
+  }();
+  if (!Provider)
+    return Opts;
----------------
Inverting the if(Provider) check isn't clearer IMO, and it's also not an optimization (`return Opts` is a copy, vs `NewOpts = Opts` is a copy + `return NewOpts` is a NVRO no-op).

Up to you, but I prefer the previous formulation (with the extra copy of defaultopts added at the top)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150257/new/

https://reviews.llvm.org/D150257