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