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

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

Thanks for the comments!



================
Comment at: clangd/ClangdLSPServer.cpp:284
+  std::string ResultUri = "";
+  if (Result) {
+    auto U = URI::create(*Result);
----------------
sammccall wrote:
> 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
That makes a lot of sense. Thanks for the suggestion!


================
Comment at: clangd/Protocol.h:51
 
-struct URI {
-  std::string uri;
+struct URIWithFile {
+  URI uri;
----------------
sammccall wrote:
> 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.
Good idea!


================
Comment at: clangd/URI.h:32
+  // By default, create a simplest valid file URI.
+  URI() : Scheme("file") {}
+
----------------
sammccall wrote:
> 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?
Yes!


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D42419