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