This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Handle absolute/relative path specifications in Config
ClosedPublic

Authored by kadircet on Oct 27 2020, 3:05 PM.

Details

Summary

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.

Diff Detail

Event Timeline

kadircet created this revision.Oct 27 2020, 3:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 27 2020, 3:05 PM
kadircet requested review of this revision.Oct 27 2020, 3:05 PM

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.

kadircet updated this revision to Diff 301280.Oct 28 2020, 7:27 AM
kadircet marked 9 inline comments as done.
  • 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.

sammccall accepted this revision.Oct 28 2020, 7:39 AM
sammccall added inline comments.
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)?
This would avoid the duplication

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?
(the abbreviation isn't terrible but consistency is nice)

123

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)

This revision is now accepted and ready to land.Oct 28 2020, 7:39 AM
kadircet updated this revision to Diff 301311.Oct 28 2020, 9:29 AM
kadircet marked 6 inline comments as done.
  • Make Directory a parameter of fromYAMLFile.
  • Factor out configRelative logic to a helper.
kadircet added inline comments.Oct 29 2020, 2:54 AM
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)

sammccall added inline comments.Oct 29 2020, 3:09 AM
clang-tools-extra/clangd/ConfigCompile.cpp
51

not that if Path == FragmentDir you're going to return "" which means "not under".
I don't think this can happen yet, but it's a trap for the future...

You might want the last line to be "return FragmentDir.empty() ? "." : Path".

kadircet updated this revision to Diff 301842.Oct 30 2020, 2:44 AM
kadircet marked an inline comment as done.
  • 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.
You don't want configRelative("","") to be ".".

So either that or handle the FragmentDir-is-empty case upfront.

170

nit: why the rename rather than just capturing?

kadircet marked 2 inline comments as done.Oct 30 2020, 8:30 AM
kadircet added inline comments.
clang-tools-extra/clangd/ConfigCompile.cpp
170

because it is actually a member of FragmentCompiler and not a scoped variable :/

kadircet updated this revision to Diff 301912.Oct 30 2020, 8:30 AM
  • Explicitly handle FragmentDir.empty() case by returning Path as-is.
This revision was landed with ongoing or failed builds.Nov 3 2020, 12:46 PM
This revision was automatically updated to reflect the committed changes.