[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 {
----------------
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.


================
Comment at: clang-tools-extra/clangd/TidyProvider.cpp:61-62
+  const ThreadsafeFS &FS;
+  std::string RelPath;
+  std::chrono::steady_clock::duration MaxStaleness;
 
----------------
Should these both be const?


================
Comment at: clang-tools-extra/clangd/TidyProvider.cpp:76
+    namespace path = llvm::sys::path;
+    assert(llvm::sys::path::is_absolute(AbsPath));
 
----------------
Given path has been brought into scope in the line above can probably remove excess qualifiers.


================
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;
----------------
How does this work with `..` components. Or does clangd ensure all absolute paths have those removed?


================
Comment at: clang-tools-extra/clangd/TidyProvider.cpp:113
+        OptionStack.push_back(std::move(*Config));
+        if (!OptionStack.back().InheritParentConfig)
+          break;
----------------
This will result in incorrect behaviour if a config specifies `InheritParentConfig` as `false`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92133