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

[Aleksandr Platonov via Phabricator via cfe-commits]

ArcsinX marked 2 inline comments as done.
ArcsinX 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))
----------------
kbobyrev wrote:
> 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.
So, I have made no changes here


================
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));
----------------
kbobyrev wrote:
> 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.
Add more comments in try to explain, that `Path` is a relative path here and it could be  with any slashes, so convert it to native to make sure it's compatible with the current system.


================
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());
----------------
kbobyrev wrote:
> 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).
Thanks for advice, fixed


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89529