This introduces a mechanism for providers to interpret paths specified
in a fragment either as absolute or relative to fragment location.
This information should be used during compile stage to handle blocks correctly.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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
clang-tools-extra/clangd/ConfigCompile.cpp | ||
---|---|---|
289 | 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... | |
clang-tools-extra/clangd/ConfigFragment.h | ||
94 | 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." | |
95 | 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. | |
96 | Not sure "Fragment" adds anything here - Directory is a little vaguer than ideal but FragmentDirectory just seems... longer. | |
98 | Why is this separate from FragmentDirectory? (i.e. when would is it not FragmentDirectory.empty() ? Absolute : Relative) | |
clang-tools-extra/clangd/ConfigProvider.cpp | ||
37 | 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 | |
123 | 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?) | |
clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp | ||
126 | can we use a slightly less degenerate example to verify that we're applying the regex to the correct string? | |
127 | please use testPath() to generate fake paths. I think this would have caught path-handling bugs on windows. | |
150 | can we fold this into the test above? it seems pretty duplicative. |
- Drop PathSpec, deduce it from emptyness of FragmentDirectory.
- Make tests more homogenous.
- Add path cannonicalization to compile stage.
clang-tools-extra/clangd/ConfigProvider.cpp | ||
---|---|---|
123 | 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. |
clang-tools-extra/clangd/ConfigCompile.cpp | ||
---|---|---|
87 | nit: associated with again (or just "normalized Fragment::SourceInfo::Directory") | |
173 | maybe pull out a method Optional<StringRef> configRelative(StringRef)? | |
179 | not needed as you've ensured FragmentDir ends with a slash | |
clang-tools-extra/clangd/ConfigProvider.cpp | ||
67 | nit: Directory matching Fragment.Source.Directory? | |
123 | Yes, I'd prefer this to be a parameter (I suppose the dir seems slightly cleaner than a boolean). (On the other hand, the fact that RelFileProvider *does* set Dir isn't a coincidence, it seems OK to hardcode that) |
- Make Directory a parameter of fromYAMLFile.
- Factor out configRelative logic to a helper.
clang-tools-extra/clangd/ConfigCompile.cpp | ||
---|---|---|
173 | Went with just StringRef as otuput and made empty imply a missmatch (i.e. being out-of-tree) |
clang-tools-extra/clangd/ConfigCompile.cpp | ||
---|---|---|
51 | not that if Path == FragmentDir you're going to return "" which means "not under". You might want the last line to be "return FragmentDir.empty() ? "." : Path". |
- Return "." if Path == FragmentDir in conifgRelative.
clang-tools-extra/clangd/ConfigCompile.cpp | ||
---|---|---|
51 | i think you meant Path.empty() ? "." : Path, right? |
Still LG but a few more nits
clang-tools-extra/clangd/ConfigCompile.cpp | ||
---|---|---|
46 | you should document+verify what this does if FragmentDir is empty, since we are triggering that case | |
51 | oops, no I think I meant (Path.empty() && !FragmentDir.empty() ? "." : Path. So either that or handle the FragmentDir-is-empty case upfront. | |
170 | nit: why the rename rather than just capturing? |
clang-tools-extra/clangd/ConfigCompile.cpp | ||
---|---|---|
170 | because it is actually a member of FragmentCompiler and not a scoped variable :/ |
you should document+verify what this does if FragmentDir is empty, since we are triggering that case