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