[PATCH] D89529: [clangd][remote] Add Windows paths support
Aleksandr Platonov via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Oct 16 05:52:52 PDT 2020
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
More information about the cfe-commits
mailing list