[PATCH] D42419: [clangd] Use new URI with scheme support in place of the existing LSP URI

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clangd/ClangdLSPServer.cpp:283
   llvm::Optional<Path> Result = Server.switchSourceHeader(Params.uri.file);
-  std::string ResultUri;
-  reply(C, Result ? URI::fromFile(*Result).uri : "");
+  std::string ResultUri = "";
+  if (Result) {
----------------
hmm, I think you want to replyerror for unexpected cases.
maybe: 

  if (ResultUri) {
    if (auto U = URI::create(*Result))
       reply(C, U->toString());
    else
      replyError(C, ErrorCode::InternalError, llvm::toString(U.takeError()));
  } else
    reply(C, "");


================
Comment at: clangd/ClangdLSPServer.cpp:284
+  std::string ResultUri = "";
+  if (Result) {
+    auto U = URI::create(*Result);
----------------
But basically I think this shows that the API is awkward. We should have a way to create a file URI from an absolute path that asserts rather than returning expected.
I'd suggest removing the default "file" scheme from create(), and adding createFile(abspath) that returns URI


================
Comment at: clangd/Protocol.h:51
 
-struct URI {
-  std::string uri;
+struct URIWithFile {
+  URI uri;
----------------
URIForFile? "withfile" doesn't really capture that they're related


================
Comment at: clangd/Protocol.h:51
 
-struct URI {
-  std::string uri;
+struct URIWithFile {
+  URI uri;
----------------
sammccall wrote:
> URIForFile? "withfile" doesn't really capture that they're related
Hmm actually, what about just `struct URIForFile { std:string AbsPath; }`
fromJSON and toJSON would do the marshalling to URI, but internally we just want the path, right?

This also gives us the usual easy null state.


================
Comment at: clangd/URI.h:32
+  // By default, create a simplest valid file URI.
+  URI() : Scheme("file") {}
+
----------------
ioeric wrote:
> same here. There are many default constructions of structures that contain URIs in ClangdLSPServer.cpp...
Does the struct-with-just-an-abspath idea address this?


================
Comment at: clangd/URI.h:45
+  /// Create a URI from unescaped scheme+authority+body.
+  static llvm::Expected<URI> create(llvm::StringRef Scheme,
+                                    llvm::StringRef Authority,
----------------
oh, sorry I missed this in the first code review...
It's really confusing to have `create(str, str, str)` do simple initialization that can't really fail, and `create(str,str)` do complicated plugin logic that can fail in lots of ways.

Can you change the first to be a constructor and just assert on the needed invariants?
(Or failing that `createFromComponents`, but I can't imagine a use case where you have components but don't know they're correct)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D42419