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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
