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
