sammccall added a comment.
Thanks for doing this! And sorry about the shaky windows support...
(There are potentially other lurking issues due to filenames being used as keys
internally, particularly case-insensitivity issues...)
================
Comment at: clang-tools-extra/clangd/URI.cpp:29
+bool isWindowsPath(llvm::StringRef Path) {
+ return Path.size() > 1 && llvm::isAlpha(Path[0]) && Path[1] == ':';
----------------
the UNC paths are also basically a windows thing, can we have
hasWindowsDriveLetter and isWindowsNetworkPath (or isWindowsUNCPath)?
================
Comment at: clang-tools-extra/clangd/URI.cpp:34
+bool isNetworkPath(llvm::StringRef Path) {
+ llvm::StringRef Sep = llvm::sys::path::get_separator();
+ return Path.consume_front(Sep) && Path.consume_front(Sep) && !Path.empty();
----------------
conventionally LLVM tools accept both slashes on windows and `/` on other
platforms - i.e. we should probably treat `//foo/bar` as a valid network path
on windows unless there's a good reason not to.
So `Path.size() > 2 && llvm::sys::path::is_separator(Path[0]) &&
llvm::sys::path::is_separator(Path[1])`, I guess
================
Comment at: clang-tools-extra/clangd/URI.cpp:52
+ llvm::SmallString<128> Path;
+ // For Windows UNC paths e.g. \\server\share
+ if (!Authority.empty())
----------------
These comments could be extended a little and be even more useful:
`Windows UNC paths e.g. \\server\share <=> file://server/share`
================
Comment at: clang-tools-extra/clangd/URI.cpp:56
// For Windows paths e.g. /X:
- if (Body.size() > 2 && Body[0] == '/' && Body[2] == ':')
+ if (isWindowsPath(Body.substr(1)))
Body.consume_front("/");
----------------
this raises the question: what if both conditions are true?
file://abc/c:/foo
`\\abc\c:` seems much more plausible than the current `\\abcc:`, so I think
this should be `else if`.
================
Comment at: clang-tools-extra/clangd/URI.cpp:67
+ llvm::StringRef Authority;
+ llvm::StringRef Root = llvm::sys::path::root_name(AbsolutePath);
+ // For Windows UNC paths e.g. \\server\share
----------------
FWIW, I think we were previously parsing drive letters even on unix, and will
no longer do so.
I don't think this is a real problem though.
================
Comment at: clang-tools-extra/clangd/unittests/URITests.cpp:149
+TEST(URITest, ResolveUNC) {
+#ifdef _WIN32
----------------
Could we add a test that the old four-slash version can still be resolved to a
path?
It probably worked by accident - we probably weren't thinking about UNC paths
at all. But someone still might depend on it now. I don't think this patch
breaks it, just want to check.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D84172/new/
https://reviews.llvm.org/D84172
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
