[PATCH] D42735: [clangd] Add a test URI scheme for lit tests to unbreak platform-specific URI failures.

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Thanks for fixing this once and for all!



================
Comment at: clangd/ClangdLSPServer.cpp:23
+/// \brief Supports a test URI scheme with relaxed constraints for lit tests.
+/// The path in a test URi will be combined with a platform-specific fake
+/// directory to form an absolute path. For example, test:///a.cpp is resolved
----------------
nit: URi -> URI


================
Comment at: clangd/ClangdLSPServer.cpp:28
+public:
+  static const char *Scheme;
+
----------------
nit: just inline this at the registry site? we shouldn't be using it elsewhere


================
Comment at: clangd/ClangdLSPServer.cpp:47
+    llvm::SmallVector<char, 16> Path(Body.begin(), Body.end());
+    auto Err = fs::make_absolute(TestDir, Path);
+    assert(!Err);
----------------
on windows you're passing `C:\clangd-test` and `/foo.cpp` here.
I think you should probably call `native` on the body before passing it here (and no need to do it after)


================
Comment at: clangd/Protocol.cpp:36
+    if (U->scheme() != "file" && U->scheme() != "test") {
+      log(Context::empty(), "Clangd only supports 'file' (and 'test' for lit "
+                            "tests) URI scheme for workspace files: " +
----------------
I'd suggest not mentioning 'test' in the log message, and just making this a comment.
If someone passes e.g. http://, mentioning test probably adds more confusion than it resolves.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D42735