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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks for doing this!

Main points:

- I think we should have one high-level concept of "the optional associated directory" rather than two low-level ones of "the location of the config file" and "should we use the location to form relative paths"
- We need to be careful about windows vs posix paths



================
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:277
 
-  FragmentCompiler{*Result, D, Source.Manager.get()}.compile(std::move(*this));
+  FragmentCompiler{*Result, D, Source.Manager.get(), Source}.compile(
+      std::move(*this));
----------------
passing a reference to part of `this` and then separately `this` by rvalue when the reference is live feels... fragile.

Maybe have compile() assign to a member stringref instead? Or even a string and do the path normalization at that point...


================
Comment at: clang-tools-extra/clangd/ConfigFragment.h:94
     llvm::SMLoc Location;
+    /// Absolute path to directory containing the fragment. Doesn't contain the
+    /// trailing slash.
----------------
directory the fragment is associated with? At this level, I don't think we should care whether it's actually a file that lives there, and I don't see why the global config would want to set this.

We should also say what this is for: "Relative paths mentioned the fragment are resolved against this directory."


================
Comment at: clang-tools-extra/clangd/ConfigFragment.h:95
+    /// Absolute path to directory containing the fragment. Doesn't contain the
+    /// trailing slash.
+    std::string FragmentDirectory;
----------------
Paths in a fragment are conventionally Posix paths, so this dir should be one too before we can use string prefixes. It's also more convenient if it has a trailing slash (current code has a bug where /foo matches /foo2).

This could be handled by the creator of SourceInfo, or we could accept native paths here (with/without slashes) and normalize at the start of compilation. Latter is probably slightly less error-prone.


================
Comment at: clang-tools-extra/clangd/ConfigFragment.h:96
+    /// trailing slash.
+    std::string FragmentDirectory;
+    /// Describes how paths specified in this fragment should be treated.
----------------
Not sure "Fragment" adds anything here - Directory is a little vaguer than ideal but FragmentDirectory just seems... longer.


================
Comment at: clang-tools-extra/clangd/ConfigFragment.h:98
+    /// Describes how paths specified in this fragment should be treated.
+    enum PathSpecification {
+      Absolute,
----------------
Why is this separate from FragmentDirectory?

(i.e. when would is it not `FragmentDirectory.empty() ? Absolute : Relative`)



================
Comment at: clang-tools-extra/clangd/ConfigProvider.cpp:36
+  void fillCacheFromDisk(llvm::vfs::FileSystem &FS, DiagnosticCallback DC,
+                         Fragment::SourceInfo::PathSpecification PathSpec) {
     CachedValue.clear();
----------------
the associated directory is invariant, like the path - consider making it a member set at construction time instead of plumbing it through all the functions


================
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;
----------------
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?)


================
Comment at: clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp:122
+    Fragment Frag;
+    Frag.If.PathMatch.emplace_back(".*");
+    Frag.Source.FragmentDirectory = "/baz";
----------------
can we use a slightly less degenerate example to verify that we're applying the regex to the correct string?


================
Comment at: clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp:123
+    Frag.If.PathMatch.emplace_back(".*");
+    Frag.Source.FragmentDirectory = "/baz";
+    return Frag;
----------------
please use testPath() to generate fake paths.

I think this would have caught path-handling bugs on windows.


================
Comment at: clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp:146
+
+TEST_F(ConfigCompileTests, PathSpecExclude) {
+  const Fragment BaseFrag = [] {
----------------
can we fold this into the test above? it seems pretty duplicative.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90270