kbobyrev added inline comments.
================
Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:61
+ if (IndexedProjectRoot.empty())
+ return static_cast<std::string>(Result);
+ llvm::sys::path::replace_path_prefix(Result, IndexedProjectRoot, "");
----------------
sammccall wrote:
> static_cast is a pretty surprising/uncommon way to write this conversion....
> I'd suggest either Result.str.str() or std::string(result)
Okay, sure! I just thought `.str().str()` looks really weird, but `std::string`
is probably a good choice.
================
Comment at: clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp:43
for (auto &Sym : Symbols) {
- const auto ProtobufMeessage = toProtobuf(Sym);
- const auto SymToProtobufAndBack = fromProtobuf(ProtobufMeessage, &Strings);
+ const auto ProtobufMessage = toProtobuf(Sym, "");
+ const auto SymToProtobufAndBack = fromProtobuf(
----------------
sammccall wrote:
> Is passing the empty string here actually valid? I think you probably want to
> pass testRoot() or some unixified equivalent
Why testroot? That's what I'm stripping from URI body, the URI is
`unittest:///TestTU.h`, hence I'm stripping from `/TestTU.h`, there is no
`/clangd-test` there. Could you elaborate?
================
Comment at: clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp:45
+ const auto SymToProtobufAndBack = fromProtobuf(
+ ProtobufMessage, &Strings, std::string(testRoot()) + '/', "unittest");
EXPECT_TRUE(SymToProtobufAndBack.hasValue());
----------------
sammccall wrote:
> this is `testPath("unittest")`, isn't it?
>
> I mean, forward vs backslashes are going to differ, but the current
> expression is going to produce `C:\test/unittest` which doesn't seem better.
I don't understand: `std::string(testRoot()) + '/'` is `/clangd-test/` on unix
and `testPath("unittest")` is `/clangd-test/unittest`. I understand the slashes
argument, but could you please elaborate on the former?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D82938/new/
https://reviews.llvm.org/D82938
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
