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