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

[Ilya Golovenko via Phabricator via cfe-commits]

ilya-golovenko marked 2 inline comments as done.
ilya-golovenko added inline comments.


================
Comment at: clang-tools-extra/clangd/URI.cpp:29
 
+bool isWindowsPath(llvm::StringRef Path) {
+  return Path.size() > 1 && llvm::isAlpha(Path[0]) && Path[1] == ':';
----------------
sammccall wrote:
> the UNC paths are also basically a windows thing, can we have hasWindowsDriveLetter and isWindowsNetworkPath (or isWindowsUNCPath)?
This kind of network paths are also supported on Unix/Linux system, e.g. `//hostname/path/file.txt` and `isNetworkPath` will handle those as well.  For example, samba and samba client support such paths. RFC 8089 calls them "non-local files" with unspecified type of protocol to access the file, i.e. it is not necessary a UNC path. Does it make sense to continue supporting Linux/Unix version of network path?


================
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();
----------------
sammccall wrote:
> 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
I tried to prevent treating paths like `/\path` or `\/path` as network paths. Maybe the following variant will work:

```
bool isNetworkPath(llvm::StringRef Path) {
 return Path.size() > 2 && Path[0] == Path[1] && llvm::sys::path::is_separator(Path[0]);
}
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84172/new/

https://reviews.llvm.org/D84172