kbobyrev requested changes to this revision. kbobyrev added a comment. This revision now requires changes to proceed.
In D89529#2337197 <https://reviews.llvm.org/D89529#2337197>, @ArcsinX wrote: > In D89529#2334517 <https://reviews.llvm.org/D89529#2334517>, @kbobyrev wrote: > >> The solution would be to just convert to the POSIX path instead of asserting >> it. > > I have updated the patch according to this advice. > It seems to me it looks more consistent now, thank you. Thank you for noticing it and taking action! The patch looks good, there are few comments that I left but it should be good to go afterwards. ================ Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:53 : Strings(Arena) { + auto PosixSeparator = + llvm::sys::path::get_separator(llvm::sys::path::Style::posix); ---------------- Please explicitly spell out `StringRef` here, otherwise visually it is appealing to put `const auto` there but it would be a `StringRef` which is no obvious. ================ 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)) ---------------- 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. ================ 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)); ---------------- 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. ================ Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:332 + Result, llvm::sys::path::Style::windows)); + return Result.str().str(); } ---------------- nit: `.str().str()` is a bit unfortunate, I have had some discussions about this in the past and I guess explicit conversion via `std::string(...)` looks a bit cleaner. ================ 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()); ---------------- Why remove this test? 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
