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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits