[PATCH] D82938: [clangd] Implement path and URI translation for remote index

[Kirill Bobyrev via Phabricator via cfe-commits]

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