This is an archive of the discontinued LLVM Phabricator instance.

[clangd][lit] Update document-link.test to respect custom resource-dir locations
ClosedPublic

Authored by kadircet on Oct 2 2020, 1:13 AM.

Diff Detail

Event Timeline

kadircet created this revision.Oct 2 2020, 1:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 2 2020, 1:13 AM
kadircet requested review of this revision.Oct 2 2020, 1:13 AM
sammccall accepted this revision.Oct 2 2020, 2:10 AM
sammccall added inline comments.
clang-tools-extra/clangd/test/document-link.test
1

nit: i'd drop "to reach resource_dir" to get this to fit on one line
(Remembering that this quirk is almost always uninteresting to people reading this test)

This revision is now accepted and ready to land.Oct 2 2020, 2:10 AM
kadircet updated this revision to Diff 295779.Oct 2 2020, 3:23 AM
  • update comment
This revision was landed with ongoing or failed builds.Oct 2 2020, 3:25 AM
This revision was automatically updated to reflect the committed changes.
ArcsinX added a subscriber: ArcsinX.Oct 2 2020, 3:34 AM

Just a question.
On Windows %resource_dir/.. looks a bit inconsistent after substitution (d:\llvm-project\build\lib\clang\12.0.0\include/..), but this test passes.
As far as Windows accepts forward and back slashes, I expect something like this %/resource_dir/.. to be consistent across all platforms.
Are there any plans to add %/resource_dir substitution as it was done for %T, %s, etc?

not that i know of.

As far as Windows accepts forward and back slashes

Note we don't rely on windows itself for this support.
All access through llvm::sys::fs APIs on windows ultimately goes through widenPath to convert to UTF-16, and this substitutes slashes.
So LLVM tools do always support / on windows and it's fairly common to rely on this for tests (abstraction is hard in lit tests).

As far as Windows accepts forward and back slashes

Note we don't rely on windows itself for this support.
All access through llvm::sys::fs APIs on windows ultimately goes through widenPath to convert to UTF-16, and this substitutes slashes.
So LLVM tools do always support / on windows and it's fairly common to rely on this for tests (abstraction is hard in lit tests).

I am not sure that understood you correctly, but widenPath does nothing for me, if I pass mixed-slash path to it.
Code:

std::string From("C:\\a/b\\c");
SmallVector<wchar_t, 128> To;
llvm::sys::windows::widenPath(From, To);
std::wcerr << To.data();

Output:

C:\a/b\c

So, if we imagine that Windows does not support /, then paths with / could not be opened with llvm::sys::fs API.

As far as Windows accepts forward and back slashes

Note we don't rely on windows itself for this support.
All access through llvm::sys::fs APIs on windows ultimately goes through widenPath to convert to UTF-16, and this substitutes slashes.
So LLVM tools do always support / on windows and it's fairly common to rely on this for tests (abstraction is hard in lit tests).

I am not sure that understood you correctly, but widenPath does nothing for me, if I pass mixed-slash path to it.

Ack, you're right. I've looked at this before, and was looking at this line:
https://github.com/llvm/llvm-project/blob/master/llvm/lib/Support/Windows/Path.inc#L103
But in the common case, we've already exited by then, so you're absolutely right...

Code:

std::string From("C:\\a/b\\c");
SmallVector<wchar_t, 128> To;
llvm::sys::windows::widenPath(From, To);
std::wcerr << To.data();

Output:

C:\a/b\c

So, if we imagine that Windows does not support /, then paths with / could not be opened with llvm::sys::fs API.

But at a higher level, ISTM the purpose of widenPath is exactly to produce a path that the win32 APIs will accept, including if the input has / (as shown by the long-path case I was confused by).
So the contract of the llvm functions ensuring that forward slashes are safe still seems to be there.