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

[Nathan James via Phabricator via cfe-commits]

njames93 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 {
----------------
sammccall wrote:
> 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.
That sounds like a good resolve. I'm guessing in the common case where the config file doesn't change, this would never mutate. 


================
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;
----------------
sammccall wrote:
> njames93 wrote:
> > How does this work with `..` components. Or does clangd ensure all absolute paths have those removed?
> Right, paths are always canonical.
If that's the case, aren't these checks redundant. Maybe put some asserts in so we don't pay for them in release builds


================
Comment at: clang-tools-extra/clangd/TidyProvider.cpp:113
+        OptionStack.push_back(std::move(*Config));
+        if (!OptionStack.back().InheritParentConfig)
+          break;
----------------
sammccall wrote:
> 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?)
I'm honestly not sure. Personally I don't think it should belong in ClangTidyOptions. Its only used when reading configuration from files(or command line). Maybe I'll rustle something up for that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92133