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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev added inline comments.


================
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))
----------------
ArcsinX wrote:
> kbobyrev wrote:
> > 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.
> But `llvm::sys::path::convert_to_slash()` returns `std::string`.  
> Could you give me an advice how to copy `std::string` into `llvm::SmallString<256>` here?
> 
> E.g. the following code looks strange for me
> ```
>   llvm::Optional<llvm::SmallString<256>> RemoteIndexRoot;
> 
> ....
> 
>     this->RemoteIndexRoot = llvm::SmallString<256>(llvm::sys::path::convert_to_slash(
>         RemoteIndexRoot, llvm::sys::path::Style::windows));
> ```
> 
Hmm, yeah right nevermind, this should be OK for this patch, I'll deal with this later.


================
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());
----------------
ArcsinX wrote:
> kbobyrev wrote:
> > Why remove this test?
> As far as I understand, idea of this code is to use absolute path (i.e. /usr/local/user/home/project/Header.h) to be relative to "/". I do not know how to make this on Windows.
> 
> Also, Marshaller ProtobufMarshaller(convert_to_slash("/"), convert_to_slash("/")); leads to assertions on Windows:
> ```
>     assert(llvm::sys::path::is_absolute(RemoteIndexRoot));
> ...
>     assert(llvm::sys::path::is_absolute(LocalIndexRoot));
> ```
> 
Ah, I see now, makes sense! Yeah, okay then in this case this should be something like `testPath("project/Header.h")` which will be absolut path (what we want) and also relative to `testPath("")` (test path root).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89529