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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev requested changes to this revision.
kbobyrev added a comment.
This revision now requires changes to proceed.

Also, please hit "Done" on the comments you resolved so that it's easier to track which ones are fixed, otherwise it's hard to follow :(

The patch looks good now, almost ready to go: comments are not really clear right now, I suggested possible alternatives.



================
Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:96
   for (const auto &Path : Message->proximity_paths()) {
+    // Create an absolute path from `Path` using the prefix `RemoteIndexRoot`.
     llvm::SmallString<256> LocalPath = llvm::StringRef(*RemoteIndexRoot);
----------------
We normally don't escape variable names a-la Markdown.

Right now it's no very clear what it means, maybe just `Construct full path using local prefix.` or optionally just remove this one.


================
Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:100
+    // `Path` could be in the non-native form, thus the created absolute path
+    // could be incompatible with the local system, so fix this.
+    llvm::sys::path::native(LocalPath);
----------------
Maybe just `FuzzyFindRequest requires proximity paths to have platform-native format in order for SymbolIndex to process the query correctly.`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89529