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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev marked 2 inline comments as done.
kbobyrev added inline comments.


================
Comment at: clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp:25
 
+const char *unittestURIToFilesystem(const char *UnittestURI,
+                                    llvm::UniqueStringSaver &Strings) {
----------------
sammccall wrote:
> The File scheme is special. Using a different URI scheme here when we only support `file` in production is confusing and seems like a last resort to be used when we can't make it work any other way.
> (For example we use it in lit tests because we must specify input filenames and the presence/absence of drive letters caused problems)
> What's being solved there that the much smaller hammer of testPath() plus a local testPathURI() helper can't solve?
This is dealing with the consequences of test infrastructure being set up in a way that makes all URIs in Symbols and Refs use "unittest" scheme, this helper simply translates those URIs into "file" URIs which are the ones being supported in production. I can not understand why this is not desirable but I'll just clear all URIs and leave those two tests as sanity checks.


================
Comment at: clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp:130
+  EXPECT_EQ(Serialized.location().file_path(), RelativePath);
+  // Local index prefix should have UNIX slashes since the relative path in
+  // Protobuf message will.
----------------
sammccall wrote:
> hmm, I don't think that's the reason.
> Either that's the contract of fromProtobuf, or it's not required and we shouldn't do it.
Ah, `llvm::sys::path::append` already does that for me. Okay, makes sense to remove this then.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82938/new/

https://reviews.llvm.org/D82938