[PATCH] D90270: [clangd] Handle absolute/relative path specifications in Config

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:75
   llvm::SourceMgr *SourceMgr;
+  // Directory containing the fragment. Absolute path with forward slashes, with
+  // a trailing slash or empty for user-config fragments.
----------------
nit: associated with again (or just "normalized Fragment::SourceInfo::Directory")


================
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:162
               return false;
+            llvm::StringRef Path = P.Path;
+            // Ignore the file if it is not nested under Fragment and strip the
----------------
maybe pull out a method `Optional<StringRef> configRelative(StringRef)`?
This would avoid the duplication


================
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:168
+            if (!FragmentDir.empty())
+              Path.consume_front("/");
             return llvm::any_of(*PathMatch, [&](const llvm::Regex &RE) {
----------------
not needed as you've ensured FragmentDir ends with a slash


================
Comment at: clang-tools-extra/clangd/ConfigProvider.cpp:67
+  // Directory associated with this fragment.
+  std::string Dir;
 
----------------
nit: Directory matching Fragment.Source.Directory?
(the abbreviation isn't terrible but consistency is nice)


================
Comment at: clang-tools-extra/clangd/ConfigProvider.cpp:124
       std::vector<CompiledFragment> Result;
-      Cache.read(FS, DC, P.FreshTime, Result);
+      Cache.read(FS, DC, P.FreshTime, Fragment::SourceInfo::Absolute, Result);
       return Result;
----------------
kadircet wrote:
> sammccall wrote:
> > this seems like a surprising place to encode this choice (why is fromYAMLFile coupled to the idea that it's a global config file not associated with any dir?)
> It was mostly because the only user of `fromYAMLFile` was user-config provider. Would you rather have this as an additional parameter instead?
> 
> I can't see any other way of updating that information directly from the user, apart from changing the interface of `Provider` to have some sort of `setPathSpec` method.
Yes, I'd prefer this to be a parameter (I suppose the dir seems slightly cleaner than a boolean).
The fact that user-config is the only user seems like a coincidence, I'd rather express the decision at that level.

(On the other hand, the fact that RelFileProvider *does* set Dir isn't a coincidence, it seems OK to hardcode that)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90270