kbobyrev requested changes to this revision.
kbobyrev added a comment.
This revision now requires changes to proceed.
Also, please hit "Done" on the comments you resolved so that it's easier to
track which ones are fixed, otherwise it's hard to follow :(
The patch looks good now, almost ready to go: comments are not really clear
right now, I suggested possible alternatives.
================
Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:96
for (const auto &Path : Message->proximity_paths()) {
+ // Create an absolute path from `Path` using the prefix `RemoteIndexRoot`.
llvm::SmallString<256> LocalPath = llvm::StringRef(*RemoteIndexRoot);
----------------
We normally don't escape variable names a-la Markdown.
Right now it's no very clear what it means, maybe just `Construct full path
using local prefix.` or optionally just remove this one.
================
Comment at:
clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:100
+ // `Path` could be in the non-native form, thus the created absolute path
+ // could be incompatible with the local system, so fix this.
+ llvm::sys::path::native(LocalPath);
----------------
Maybe just `FuzzyFindRequest requires proximity paths to have platform-native
format in order for SymbolIndex to process the query correctly.`.
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
