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

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

> In D89529#2334517 <https://reviews.llvm.org/D89529#2334517>, @kbobyrev wrote:
>
>> The solution would be to just convert to the POSIX path instead of asserting it.
>
> I have updated the patch according to this advice.
> It seems to me it looks more consistent now, thank you.

Thank you for noticing it and taking action! The patch looks good, there are few comments that I left but it should be good to go afterwards.



================
Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:53
     : Strings(Arena) {
+  auto PosixSeparator =
+      llvm::sys::path::get_separator(llvm::sys::path::Style::posix);
----------------
Please explicitly spell out `StringRef` here, otherwise visually it is appealing to put `const auto` there but it would be a `StringRef` which is no obvious.


================
Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:59
+        RemoteIndexRoot, llvm::sys::path::Style::windows);
+    llvm::StringRef Path(*this->RemoteIndexRoot);
+    if (!Path.endswith(PosixSeparator))
----------------
nit: maybe it's time to change type of `RemoteIndexRoot` field to `llvm::SmallString<256>` and use `!this->RemoteIndexRoot.endswith(PosixSeparator)` instead of additional variable. Not really important for this patch but I should probably do it anyway if it's not changed in this patch.


================
Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:98
     llvm::sys::path::append(LocalPath, Path);
+    llvm::sys::path::native(LocalPath);
     Result.ProximityPaths.push_back(std::string(LocalPath));
----------------
Please add a comment explaining why it is needed here: these paths are not converted to URIs and have to be specific to the server platform in order for the query to work correctly.


================
Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:332
+                       Result, llvm::sys::path::Style::windows));
+  return Result.str().str();
 }
----------------
nit: `.str().str()` is a bit unfortunate, I have had some discussions about this in the past and I guess explicit conversion via `std::string(...)` looks a bit cleaner.


================
Comment at: clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp:242
   // Add only valid headers.
-  Header.IncludeHeader = Strings.save(
-      URI::createFile("/usr/local/user/home/project/Header.h").toString());
----------------
Why remove this test?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89529