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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
