[PATCH] D84172: [clangd] Fix conversion from Windows UNC paths to file URI format.

[Sam McCall via Phabricator via cfe-commits]

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