[PATCH] D89529: [clangd][remote] Add Windows paths support

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev added a comment.

In D89529#2334475 <https://reviews.llvm.org/D89529#2334475>, @ArcsinX wrote:

> In D89529#2334407 <https://reviews.llvm.org/D89529#2334407>, @kbobyrev wrote:
>
>> Hi, thanks for flagging the issue! Could you please give more information where exactly the tests fail (e.g. logs with assertions)? This way I could understand where exactly the failure happens.
>
> With current implementation I could not pass Windows path (e.g. C:\a\b\c) to `Marshaller` constructor because of  `assert(RemoteIndexRoot ==  llvm::sys::path::convert_to_slash(RemoteIndexRoot));`, because on Windows  `llvm::sys::path::convert_to_slash(RemoteIndexRoot)` converts C:\a\b\c to C:/a/b/c

Yes, I mentioned it above. The solution would be to just convert to the POSIX path instead of asserting it.

> Another problem is that Windows URI paths are not supported without this patch (e.g. URI=file:///X:/path => Body=/X:/path, and /X:/path does not start with X:\path)

This is an interesting problem and I think this should be covered by the URI code that we have but I should check. This might have been triggered by a different issue.

> The third problem, that without conversion local Windows paths to native slashes, mixed-slash paths appears at `path::append()` calls and we again face the problem when some paths does not start with given prefix (but it should)

Yes, I mentioned it above. This is not an actual problem if all the paths within marshalling are POSIX style. There is a problem with

>> I can understand what you're trying to do but the patch introduces the behaviour which is unintended as of right now. The convention is to use POSIX paths everywhere in the Marshalling code since the end result will be a URI anyway. The patch changes it to Windows slashes which is orthogonal to this direction.
>
> Idea is to use native slashes (Windows slashes on Windows) *for local paths only* and UNIX slashes inside the wire.
>
> Just to clarify why I changed `llvm::sys::path::convert_to_slash(Result, llvm::sys::path::Style::posix)` to `llvm::sys::path::convert_to_slash(Result, llvm::sys::path::Style::windows)`:
>
> - `llvm::sys::path::convert_to_slash(Result, llvm::sys::path::Style::posix)` no-op on any platform and does nothing with '\' in Windows paths.

Yeah you're right, I missed these `convert_to_slash` semantics and that should be changed.

> - `llvm::sys::path::convert_to_slash(Result, llvm::sys::path::Style::windows)` replaces '\' with '/' on any platform.

We don't have actual filesystem paths anywhere outside `FuzzyFindRequest`, every other clangd struct stores URIs rather than paths. And when the URI is created it doesn't matter whether this path would be a Windows or POSIX, a URI is portable. Hence, the only place with native paths should be the `FuzzyFindRequest` translation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89529