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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/TidyProvider.cpp:34
+  llvm::Optional<tidy::ClangTidyOptions>
+  get(const ThreadsafeFS &TFS,
+      std::chrono::steady_clock::time_point FreshTime) const {
----------------
njames93 wrote:
> To save a copy, could this not return a const pointer to whats stored in `Value`, nullptr if `Value` is empty.
> In `DotClangTidyTree`, `OptionStack` could then store pointers instead of values.
> Considering the size of ClangTidyOptions (312 bytes in x64 with libstdc++) this is definitely a worthwhile saving.
> 
> One slight issue is I'm not sure how nicely this approach would play with the mutex.
You're right, and 312 bytes understates it - it contains strings and stuff that likely allocate on the heap when copied.

The issue with a pointer is that the underlying value may not live long enough (or to put it another way, the pointer will point to the cache slot which may be concurrently modified).

I think the fix for that is to store and return a `shared_ptr<const ClangTidyOptions>`. I'll give that a try.


================
Comment at: clang-tools-extra/clangd/TidyProvider.cpp:61-62
+  const ThreadsafeFS &FS;
+  std::string RelPath;
+  std::chrono::steady_clock::duration MaxStaleness;
 
----------------
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...)


================
Comment at: clang-tools-extra/clangd/TidyProvider.cpp:84-87
+      // Avoid weird non-substring cases like phantom "." components.
+      // In practice, Component is a substring for all "normal" ancestors.
+      if (I->end() < Parent.begin() && I->end() > Parent.end())
+        continue;
----------------
njames93 wrote:
> How does this work with `..` components. Or does clangd ensure all absolute paths have those removed?
Right, paths are always canonical.


================
Comment at: clang-tools-extra/clangd/TidyProvider.cpp:113
+        OptionStack.push_back(std::move(*Config));
+        if (!OptionStack.back().InheritParentConfig)
+          break;
----------------
njames93 wrote:
> This will result in incorrect behaviour if a config specifies `InheritParentConfig` as `false`
Whoops, thanks!
(Why is this optional<bool> rather than bool?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92133