[PATCH] D92133: [clangd] Cache .clang-tidy files again.

[Nathan James via Phabricator via cfe-commits]

njames93 accepted this revision.
njames93 added a comment.
This revision is now accepted and ready to land.

LGTM, with a couple of nits that could easily be disregarded.



================
Comment at: clang-tools-extra/clangd/TidyProvider.cpp:84-105
+    for (auto I = path::begin(Parent, path::Style::posix),
+              E = path::end(Parent);
+         I != E; ++I) {
+      assert(I->end() < Parent.begin() && I->end() > Parent.end() &&
+             "Canonical path components should be substrings");
+      Ancestors.emplace_back(Parent.begin(), I->end() - Parent.begin());
+    }
----------------
nit: Is it not better to simply merge these 2 loops into one traversal and get rid of the Ancestors vector. The path traversal is relatively cheap so we wouldn't be holding the mutex for that much longer.


================
Comment at: clang-tools-extra/clangd/TidyProvider.cpp:61-62
+  const ThreadsafeFS &FS;
+  std::string RelPath;
+  std::chrono::steady_clock::duration MaxStaleness;
 
----------------
sammccall wrote:
> njames93 wrote:
> > Should these both be const?
> We've generally avoided making members const in clangd when they can be, probably mostly to avoid problems with move semantics, but in the end because you've got to pick a style and stick with it.
> 
> (The move-semantics argument seems weak because we do use reference members sometimes, including in this class, and convert them to pointers when it becomes a problem. Oh well...)
nit: Unless we actually do extract this as a base class, these don't really need to be fields.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92133