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

[Aleksandr Platonov via Phabricator via cfe-commits]

ArcsinX added a comment.

In D89529#2334517 <https://reviews.llvm.org/D89529#2334517>, @kbobyrev wrote:

> 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.

Ok, got your point. I will think about this.
But for me it's hard to see the reason why we need to store local paths in POSIX style instead of native. From my point of view, we need to ensure that paths inside the wire are in POSIX style and nothing more. E.g. clangd converts paths extracted from LSP to native style.

>> 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.

https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/clangd/URI.cpp#L51

The reason why I put this all together is that single change leads to the same amount of tests failures (test just fails by other reason).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89529