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