[PATCH] D42419: [clangd] Use new URI with scheme support in place of the existing LSP URI
This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE323660: [clangd] Use new URI with scheme support in place
of the existing LSP URI (authored by ioeric, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D42419?vs=131802=131803#toc
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42419
Files:
clangd/ClangdLSPServer.cpp
clangd/Protocol.cpp
clangd/Protocol.h
clangd/URI.cpp
clangd/URI.h
clangd/XRefs.cpp
clangd/clients/clangd-vscode/src/extension.ts
unittests/clangd/URITests.cpp
unittests/clangd/XRefsTests.cpp
Index: unittests/clangd/URITests.cpp
===
--- unittests/clangd/URITests.cpp
+++ unittests/clangd/URITests.cpp
@@ -40,13 +40,12 @@
.str();
}
- llvm::Expected
+ llvm::Expected
uriFromAbsolutePath(llvm::StringRef AbsolutePath) const override {
auto Pos = AbsolutePath.find(TestRoot);
assert(Pos != llvm::StringRef::npos);
-return FileURI::create(
-Scheme, /*Authority=*/"",
-AbsolutePath.substr(Pos + llvm::StringRef(TestRoot).size()));
+return URI(Scheme, /*Authority=*/"",
+ AbsolutePath.substr(Pos + llvm::StringRef(TestRoot).size()));
}
};
@@ -57,30 +56,22 @@
std::string createOrDie(llvm::StringRef AbsolutePath,
llvm::StringRef Scheme = "file") {
- auto Uri = FileURI::create(AbsolutePath, Scheme);
+ auto Uri = URI::create(AbsolutePath, Scheme);
if (!Uri)
llvm_unreachable(llvm::toString(Uri.takeError()).c_str());
return Uri->toString();
}
-std::string createOrDie(llvm::StringRef Scheme, llvm::StringRef Authority,
-llvm::StringRef Body) {
- auto Uri = FileURI::create(Scheme, Authority, Body);
- if (!Uri)
-llvm_unreachable(llvm::toString(Uri.takeError()).c_str());
- return Uri->toString();
-}
-
-FileURI parseOrDie(llvm::StringRef Uri) {
- auto U = FileURI::parse(Uri);
+URI parseOrDie(llvm::StringRef Uri) {
+ auto U = URI::parse(Uri);
if (!U)
llvm_unreachable(llvm::toString(U.takeError()).c_str());
return *U;
}
TEST(PercentEncodingTest, Encode) {
- EXPECT_EQ(createOrDie("x", /*Authority=*/"", "a/b/c"), "x:a/b/c");
- EXPECT_EQ(createOrDie("x", /*Authority=*/"", "a!b;c~"), "x:a%21b%3bc~");
+ EXPECT_EQ(URI("x", /*Authority=*/"", "a/b/c").toString(), "x:a/b/c");
+ EXPECT_EQ(URI("x", /*Authority=*/"", "a!b;c~").toString(), "x:a%21b%3bc~");
}
TEST(PercentEncodingTest, Decode) {
@@ -93,8 +84,8 @@
EXPECT_EQ(parseOrDie("x:a%21b%3ac~").body(), "a!b:c~");
}
-std::string resolveOrDie(const FileURI , llvm::StringRef HintPath = "") {
- auto Path = FileURI::resolve(U, HintPath);
+std::string resolveOrDie(const URI , llvm::StringRef HintPath = "") {
+ auto Path = URI::resolve(U, HintPath);
if (!Path)
llvm_unreachable(llvm::toString(Path.takeError()).c_str());
return *Path;
@@ -110,25 +101,16 @@
}
TEST(URITest, FailedCreate) {
- auto Fail = [](llvm::Expected U) {
+ auto Fail = [](llvm::Expected U) {
if (!U) {
llvm::consumeError(U.takeError());
return true;
}
return false;
};
- // Create from scheme+authority+body:
- //
- // Scheme must be provided.
- EXPECT_TRUE(Fail(FileURI::create("", "auth", "/a")));
- // Body must start with '/' if authority is present.
- EXPECT_TRUE(Fail(FileURI::create("scheme", "auth", "x/y/z")));
-
- // Create from scheme registry:
- //
- EXPECT_TRUE(Fail(FileURI::create("/x/y/z", "no")));
+ EXPECT_TRUE(Fail(URI::create("/x/y/z", "no")));
// Path has to be absolute.
- EXPECT_TRUE(Fail(FileURI::create("x/y/z")));
+ EXPECT_TRUE(Fail(URI::create("x/y/z", "file")));
}
TEST(URITest, Parse) {
@@ -163,7 +145,7 @@
TEST(URITest, ParseFailed) {
auto FailedParse = [](llvm::StringRef U) {
-auto URI = FileURI::parse(U);
+auto URI = URI::parse(U);
if (!URI) {
llvm::consumeError(URI.takeError());
return true;
@@ -194,14 +176,14 @@
TEST(URITest, Platform) {
auto Path = getVirtualTestFilePath("x");
- auto U = FileURI::create(Path, "file");
+ auto U = URI::create(Path, "file");
EXPECT_TRUE(static_cast(U));
EXPECT_THAT(resolveOrDie(*U), Path.str());
}
TEST(URITest, ResolveFailed) {
auto FailedResolve = [](llvm::StringRef Uri) {
-auto Path = FileURI::resolve(parseOrDie(Uri));
+auto Path = URI::resolve(parseOrDie(Uri));
if (!Path) {
llvm::consumeError(Path.takeError());
return true;
Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -9,6 +9,7 @@
#include "Annotations.h"
#include "ClangdUnit.h"
#include "Matchers.h"
+#include "TestFS.h"
#include "XRefs.h"
#include "clang/Frontend/CompilerInvocation.h"
#include "clang/Frontend/PCHContainerOperations.h"
@@ -38,7 +39,9 @@
// FIXME: this is duplicated with
[PATCH] D42419: [clangd] Use new URI with scheme support in place of the existing LSP URI
ioeric updated this revision to Diff 131802.
ioeric marked an inline comment as done.
ioeric added a comment.
- addressed review comments; removed URI hack in vscode extenstion.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42419
Files:
clangd/ClangdLSPServer.cpp
clangd/Protocol.cpp
clangd/Protocol.h
clangd/URI.cpp
clangd/URI.h
clangd/XRefs.cpp
clangd/clients/clangd-vscode/src/extension.ts
unittests/clangd/URITests.cpp
unittests/clangd/XRefsTests.cpp
Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -9,6 +9,7 @@
#include "Annotations.h"
#include "ClangdUnit.h"
#include "Matchers.h"
+#include "TestFS.h"
#include "XRefs.h"
#include "clang/Frontend/CompilerInvocation.h"
#include "clang/Frontend/PCHContainerOperations.h"
@@ -38,7 +39,9 @@
// FIXME: this is duplicated with FileIndexTests. Share it.
ParsedAST build(StringRef Code) {
- auto CI = createInvocationFromCommandLine({"clang", "-xc++", "Foo.cpp"});
+ auto TestFile = getVirtualTestFilePath("Foo.cpp");
+ auto CI =
+ createInvocationFromCommandLine({"clang", "-xc++", TestFile.c_str()});
auto Buf = MemoryBuffer::getMemBuffer(Code);
auto AST = ParsedAST::Build(
Context::empty(), std::move(CI), nullptr, std::move(Buf),
Index: unittests/clangd/URITests.cpp
===
--- unittests/clangd/URITests.cpp
+++ unittests/clangd/URITests.cpp
@@ -40,13 +40,12 @@
.str();
}
- llvm::Expected
+ llvm::Expected
uriFromAbsolutePath(llvm::StringRef AbsolutePath) const override {
auto Pos = AbsolutePath.find(TestRoot);
assert(Pos != llvm::StringRef::npos);
-return FileURI::create(
-Scheme, /*Authority=*/"",
-AbsolutePath.substr(Pos + llvm::StringRef(TestRoot).size()));
+return URI(Scheme, /*Authority=*/"",
+ AbsolutePath.substr(Pos + llvm::StringRef(TestRoot).size()));
}
};
@@ -57,30 +56,22 @@
std::string createOrDie(llvm::StringRef AbsolutePath,
llvm::StringRef Scheme = "file") {
- auto Uri = FileURI::create(AbsolutePath, Scheme);
+ auto Uri = URI::create(AbsolutePath, Scheme);
if (!Uri)
llvm_unreachable(llvm::toString(Uri.takeError()).c_str());
return Uri->toString();
}
-std::string createOrDie(llvm::StringRef Scheme, llvm::StringRef Authority,
-llvm::StringRef Body) {
- auto Uri = FileURI::create(Scheme, Authority, Body);
- if (!Uri)
-llvm_unreachable(llvm::toString(Uri.takeError()).c_str());
- return Uri->toString();
-}
-
-FileURI parseOrDie(llvm::StringRef Uri) {
- auto U = FileURI::parse(Uri);
+URI parseOrDie(llvm::StringRef Uri) {
+ auto U = URI::parse(Uri);
if (!U)
llvm_unreachable(llvm::toString(U.takeError()).c_str());
return *U;
}
TEST(PercentEncodingTest, Encode) {
- EXPECT_EQ(createOrDie("x", /*Authority=*/"", "a/b/c"), "x:a/b/c");
- EXPECT_EQ(createOrDie("x", /*Authority=*/"", "a!b;c~"), "x:a%21b%3bc~");
+ EXPECT_EQ(URI("x", /*Authority=*/"", "a/b/c").toString(), "x:a/b/c");
+ EXPECT_EQ(URI("x", /*Authority=*/"", "a!b;c~").toString(), "x:a%21b%3bc~");
}
TEST(PercentEncodingTest, Decode) {
@@ -93,8 +84,8 @@
EXPECT_EQ(parseOrDie("x:a%21b%3ac~").body(), "a!b:c~");
}
-std::string resolveOrDie(const FileURI , llvm::StringRef HintPath = "") {
- auto Path = FileURI::resolve(U, HintPath);
+std::string resolveOrDie(const URI , llvm::StringRef HintPath = "") {
+ auto Path = URI::resolve(U, HintPath);
if (!Path)
llvm_unreachable(llvm::toString(Path.takeError()).c_str());
return *Path;
@@ -110,25 +101,16 @@
}
TEST(URITest, FailedCreate) {
- auto Fail = [](llvm::Expected U) {
+ auto Fail = [](llvm::Expected U) {
if (!U) {
llvm::consumeError(U.takeError());
return true;
}
return false;
};
- // Create from scheme+authority+body:
- //
- // Scheme must be provided.
- EXPECT_TRUE(Fail(FileURI::create("", "auth", "/a")));
- // Body must start with '/' if authority is present.
- EXPECT_TRUE(Fail(FileURI::create("scheme", "auth", "x/y/z")));
-
- // Create from scheme registry:
- //
- EXPECT_TRUE(Fail(FileURI::create("/x/y/z", "no")));
+ EXPECT_TRUE(Fail(URI::create("/x/y/z", "no")));
// Path has to be absolute.
- EXPECT_TRUE(Fail(FileURI::create("x/y/z")));
+ EXPECT_TRUE(Fail(URI::create("x/y/z", "file")));
}
TEST(URITest, Parse) {
@@ -163,7 +145,7 @@
TEST(URITest, ParseFailed) {
auto FailedParse = [](llvm::StringRef U) {
-auto URI = FileURI::parse(U);
+auto URI = URI::parse(U);
if (!URI) {
llvm::consumeError(URI.takeError());
return true;
@@ -194,14 +176,14 @@
TEST(URITest, Platform) {
auto Path = getVirtualTestFilePath("x");
- auto U = FileURI::create(Path, "file");
+ auto U = URI::create(Path, "file");
EXPECT_TRUE(static_cast(U));
[PATCH] D42419: [clangd] Use new URI with scheme support in place of the existing LSP URI
sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
Can you also remove the URI-encoding hack from the VSCode client?
Comment at: clangd/Protocol.cpp:35
+}
+auto Resolved = URI::resolve(*U);
+if (!Resolved) {
I think you can just check that the scheme is file and pull out the path?
we don't want to expose custom URI schemes in LSP, I think
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
[PATCH] D42419: [clangd] Use new URI with scheme support in place of the existing LSP URI
ioeric updated this revision to Diff 131775.
ioeric added a comment.
- Merge remote-tracking branch 'origin/master' into uri
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42419
Files:
clangd/ClangdLSPServer.cpp
clangd/Protocol.cpp
clangd/Protocol.h
clangd/URI.cpp
clangd/URI.h
clangd/XRefs.cpp
unittests/clangd/URITests.cpp
unittests/clangd/XRefsTests.cpp
Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -9,6 +9,7 @@
#include "Annotations.h"
#include "ClangdUnit.h"
#include "Matchers.h"
+#include "TestFS.h"
#include "XRefs.h"
#include "clang/Frontend/CompilerInvocation.h"
#include "clang/Frontend/PCHContainerOperations.h"
@@ -38,7 +39,9 @@
// FIXME: this is duplicated with FileIndexTests. Share it.
ParsedAST build(StringRef Code) {
- auto CI = createInvocationFromCommandLine({"clang", "-xc++", "Foo.cpp"});
+ auto TestFile = getVirtualTestFilePath("Foo.cpp");
+ auto CI =
+ createInvocationFromCommandLine({"clang", "-xc++", TestFile.c_str()});
auto Buf = MemoryBuffer::getMemBuffer(Code);
auto AST = ParsedAST::Build(
Context::empty(), std::move(CI), nullptr, std::move(Buf),
Index: unittests/clangd/URITests.cpp
===
--- unittests/clangd/URITests.cpp
+++ unittests/clangd/URITests.cpp
@@ -40,13 +40,12 @@
.str();
}
- llvm::Expected
+ llvm::Expected
uriFromAbsolutePath(llvm::StringRef AbsolutePath) const override {
auto Pos = AbsolutePath.find(TestRoot);
assert(Pos != llvm::StringRef::npos);
-return FileURI::create(
-Scheme, /*Authority=*/"",
-AbsolutePath.substr(Pos + llvm::StringRef(TestRoot).size()));
+return URI(Scheme, /*Authority=*/"",
+ AbsolutePath.substr(Pos + llvm::StringRef(TestRoot).size()));
}
};
@@ -57,30 +56,22 @@
std::string createOrDie(llvm::StringRef AbsolutePath,
llvm::StringRef Scheme = "file") {
- auto Uri = FileURI::create(AbsolutePath, Scheme);
+ auto Uri = URI::create(AbsolutePath, Scheme);
if (!Uri)
llvm_unreachable(llvm::toString(Uri.takeError()).c_str());
return Uri->toString();
}
-std::string createOrDie(llvm::StringRef Scheme, llvm::StringRef Authority,
-llvm::StringRef Body) {
- auto Uri = FileURI::create(Scheme, Authority, Body);
- if (!Uri)
-llvm_unreachable(llvm::toString(Uri.takeError()).c_str());
- return Uri->toString();
-}
-
-FileURI parseOrDie(llvm::StringRef Uri) {
- auto U = FileURI::parse(Uri);
+URI parseOrDie(llvm::StringRef Uri) {
+ auto U = URI::parse(Uri);
if (!U)
llvm_unreachable(llvm::toString(U.takeError()).c_str());
return *U;
}
TEST(PercentEncodingTest, Encode) {
- EXPECT_EQ(createOrDie("x", /*Authority=*/"", "a/b/c"), "x:a/b/c");
- EXPECT_EQ(createOrDie("x", /*Authority=*/"", "a!b;c~"), "x:a%21b%3bc~");
+ EXPECT_EQ(URI("x", /*Authority=*/"", "a/b/c").toString(), "x:a/b/c");
+ EXPECT_EQ(URI("x", /*Authority=*/"", "a!b;c~").toString(), "x:a%21b%3bc~");
}
TEST(PercentEncodingTest, Decode) {
@@ -93,8 +84,8 @@
EXPECT_EQ(parseOrDie("x:a%21b%3ac~").body(), "a!b:c~");
}
-std::string resolveOrDie(const FileURI , llvm::StringRef HintPath = "") {
- auto Path = FileURI::resolve(U, HintPath);
+std::string resolveOrDie(const URI , llvm::StringRef HintPath = "") {
+ auto Path = URI::resolve(U, HintPath);
if (!Path)
llvm_unreachable(llvm::toString(Path.takeError()).c_str());
return *Path;
@@ -110,25 +101,16 @@
}
TEST(URITest, FailedCreate) {
- auto Fail = [](llvm::Expected U) {
+ auto Fail = [](llvm::Expected U) {
if (!U) {
llvm::consumeError(U.takeError());
return true;
}
return false;
};
- // Create from scheme+authority+body:
- //
- // Scheme must be provided.
- EXPECT_TRUE(Fail(FileURI::create("", "auth", "/a")));
- // Body must start with '/' if authority is present.
- EXPECT_TRUE(Fail(FileURI::create("scheme", "auth", "x/y/z")));
-
- // Create from scheme registry:
- //
- EXPECT_TRUE(Fail(FileURI::create("/x/y/z", "no")));
+ EXPECT_TRUE(Fail(URI::create("/x/y/z", "no")));
// Path has to be absolute.
- EXPECT_TRUE(Fail(FileURI::create("x/y/z")));
+ EXPECT_TRUE(Fail(URI::create("x/y/z", "file")));
}
TEST(URITest, Parse) {
@@ -163,7 +145,7 @@
TEST(URITest, ParseFailed) {
auto FailedParse = [](llvm::StringRef U) {
-auto URI = FileURI::parse(U);
+auto URI = URI::parse(U);
if (!URI) {
llvm::consumeError(URI.takeError());
return true;
@@ -194,14 +176,14 @@
TEST(URITest, Platform) {
auto Path = getVirtualTestFilePath("x");
- auto U = FileURI::create(Path, "file");
+ auto U = URI::create(Path, "file");
EXPECT_TRUE(static_cast(U));
EXPECT_THAT(resolveOrDie(*U), Path.str());
}
TEST(URITest, ResolveFailed) {
auto FailedResolve =
[PATCH] D42419: [clangd] Use new URI with scheme support in place of the existing LSP URI
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
[PATCH] D42419: [clangd] Use new URI with scheme support in place of the existing LSP URI
ioeric updated this revision to Diff 131576.
ioeric marked 8 inline comments as done.
ioeric added a comment.
- Address review comments
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42419
Files:
clangd/ClangdLSPServer.cpp
clangd/Protocol.cpp
clangd/Protocol.h
clangd/URI.cpp
clangd/URI.h
clangd/XRefs.cpp
unittests/clangd/URITests.cpp
unittests/clangd/XRefsTests.cpp
Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -9,6 +9,7 @@
#include "Annotations.h"
#include "ClangdUnit.h"
#include "Matchers.h"
+#include "TestFS.h"
#include "XRefs.h"
#include "clang/Frontend/CompilerInvocation.h"
#include "clang/Frontend/PCHContainerOperations.h"
@@ -38,7 +39,9 @@
// FIXME: this is duplicated with FileIndexTests. Share it.
ParsedAST build(StringRef Code) {
- auto CI = createInvocationFromCommandLine({"clang", "-xc++", "Foo.cpp"});
+ auto TestFile = getVirtualTestFilePath("Foo.cpp");
+ auto CI =
+ createInvocationFromCommandLine({"clang", "-xc++", TestFile.c_str()});
auto Buf = MemoryBuffer::getMemBuffer(Code);
auto AST = ParsedAST::Build(
Context::empty(), std::move(CI), nullptr, std::move(Buf),
Index: unittests/clangd/URITests.cpp
===
--- unittests/clangd/URITests.cpp
+++ unittests/clangd/URITests.cpp
@@ -40,13 +40,12 @@
.str();
}
- llvm::Expected
+ llvm::Expected
uriFromAbsolutePath(llvm::StringRef AbsolutePath) const override {
auto Pos = AbsolutePath.find(TestRoot);
assert(Pos != llvm::StringRef::npos);
-return FileURI::create(
-Scheme, /*Authority=*/"",
-AbsolutePath.substr(Pos + llvm::StringRef(TestRoot).size()));
+return URI(Scheme, /*Authority=*/"",
+ AbsolutePath.substr(Pos + llvm::StringRef(TestRoot).size()));
}
};
@@ -57,30 +56,22 @@
std::string createOrDie(llvm::StringRef AbsolutePath,
llvm::StringRef Scheme = "file") {
- auto Uri = FileURI::create(AbsolutePath, Scheme);
+ auto Uri = URI::create(AbsolutePath, Scheme);
if (!Uri)
llvm_unreachable(llvm::toString(Uri.takeError()).c_str());
return Uri->toString();
}
-std::string createOrDie(llvm::StringRef Scheme, llvm::StringRef Authority,
-llvm::StringRef Body) {
- auto Uri = FileURI::create(Scheme, Authority, Body);
- if (!Uri)
-llvm_unreachable(llvm::toString(Uri.takeError()).c_str());
- return Uri->toString();
-}
-
-FileURI parseOrDie(llvm::StringRef Uri) {
- auto U = FileURI::parse(Uri);
+URI parseOrDie(llvm::StringRef Uri) {
+ auto U = URI::parse(Uri);
if (!U)
llvm_unreachable(llvm::toString(U.takeError()).c_str());
return *U;
}
TEST(PercentEncodingTest, Encode) {
- EXPECT_EQ(createOrDie("x", /*Authority=*/"", "a/b/c"), "x:a/b/c");
- EXPECT_EQ(createOrDie("x", /*Authority=*/"", "a!b;c~"), "x:a%21b%3bc~");
+ EXPECT_EQ(URI("x", /*Authority=*/"", "a/b/c").toString(), "x:a/b/c");
+ EXPECT_EQ(URI("x", /*Authority=*/"", "a!b;c~").toString(), "x:a%21b%3bc~");
}
TEST(PercentEncodingTest, Decode) {
@@ -93,8 +84,8 @@
EXPECT_EQ(parseOrDie("x:a%21b%3ac~").body(), "a!b:c~");
}
-std::string resolveOrDie(const FileURI , llvm::StringRef HintPath = "") {
- auto Path = FileURI::resolve(U, HintPath);
+std::string resolveOrDie(const URI , llvm::StringRef HintPath = "") {
+ auto Path = URI::resolve(U, HintPath);
if (!Path)
llvm_unreachable(llvm::toString(Path.takeError()).c_str());
return *Path;
@@ -110,25 +101,16 @@
}
TEST(URITest, FailedCreate) {
- auto Fail = [](llvm::Expected U) {
+ auto Fail = [](llvm::Expected U) {
if (!U) {
llvm::consumeError(U.takeError());
return true;
}
return false;
};
- // Create from scheme+authority+body:
- //
- // Scheme must be provided.
- EXPECT_TRUE(Fail(FileURI::create("", "auth", "/a")));
- // Body must start with '/' if authority is present.
- EXPECT_TRUE(Fail(FileURI::create("scheme", "auth", "x/y/z")));
-
- // Create from scheme registry:
- //
- EXPECT_TRUE(Fail(FileURI::create("/x/y/z", "no")));
+ EXPECT_TRUE(Fail(URI::create("/x/y/z", "no")));
// Path has to be absolute.
- EXPECT_TRUE(Fail(FileURI::create("x/y/z")));
+ EXPECT_TRUE(Fail(URI::create("x/y/z", "file")));
}
TEST(URITest, Parse) {
@@ -163,7 +145,7 @@
TEST(URITest, ParseFailed) {
auto FailedParse = [](llvm::StringRef U) {
-auto URI = FileURI::parse(U);
+auto URI = URI::parse(U);
if (!URI) {
llvm::consumeError(URI.takeError());
return true;
@@ -194,14 +176,14 @@
TEST(URITest, Platform) {
auto Path = getVirtualTestFilePath("x");
- auto U = FileURI::create(Path, "file");
+ auto U = URI::create(Path, "file");
EXPECT_TRUE(static_cast(U));
EXPECT_THAT(resolveOrDie(*U), Path.str());
}
TEST(URITest, ResolveFailed) {
auto
[PATCH] D42419: [clangd] Use new URI with scheme support in place of the existing LSP URI
sammccall added inline comments.
Comment at: clangd/ClangdLSPServer.cpp:283
llvm::Optional 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 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
[PATCH] D42419: [clangd] Use new URI with scheme support in place of the existing LSP URI
ioeric added inline comments.
Comment at: clangd/ClangdLSPServer.cpp:386
+ if (!U)
+llvm_unreachable(
+"onDiagnosticsReady: Expect creating URI for file to always work.");
not sure if this is the right thing to do. idea?
Comment at: clangd/URI.h:32
+ // By default, create a simplest valid file URI.
+ URI() : Scheme("file") {}
+
same here. There are many default constructions of structures that contain URIs
in ClangdLSPServer.cpp...
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
[PATCH] D42419: [clangd] Use new URI with scheme support in place of the existing LSP URI
ioeric created this revision.
ioeric added a reviewer: sammccall.
Herald added subscribers: cfe-commits, jkorous-apple, ilya-biryukov, klimek.
o Replace the existing clangd::URI with a wrapper of FileURI which also
carries a resolved file path.
o s/FileURI/URI/
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42419
Files:
clangd/ClangdLSPServer.cpp
clangd/Protocol.cpp
clangd/Protocol.h
clangd/URI.cpp
clangd/URI.h
clangd/XRefs.cpp
unittests/clangd/URITests.cpp
unittests/clangd/XRefsTests.cpp
Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -9,6 +9,7 @@
#include "Annotations.h"
#include "ClangdUnit.h"
#include "Matchers.h"
+#include "TestFS.h"
#include "XRefs.h"
#include "clang/Frontend/CompilerInvocation.h"
#include "clang/Frontend/PCHContainerOperations.h"
@@ -38,7 +39,9 @@
// FIXME: this is duplicated with FileIndexTests. Share it.
ParsedAST build(StringRef Code) {
- auto CI = createInvocationFromCommandLine({"clang", "-xc++", "Foo.cpp"});
+ auto TestFile = getVirtualTestFilePath("Foo.cpp");
+ auto CI =
+ createInvocationFromCommandLine({"clang", "-xc++", TestFile.c_str()});
auto Buf = MemoryBuffer::getMemBuffer(Code);
auto AST = ParsedAST::Build(
Context::empty(), std::move(CI), nullptr, std::move(Buf),
Index: unittests/clangd/URITests.cpp
===
--- unittests/clangd/URITests.cpp
+++ unittests/clangd/URITests.cpp
@@ -40,11 +40,11 @@
.str();
}
- llvm::Expected
+ llvm::Expected
uriFromAbsolutePath(llvm::StringRef AbsolutePath) const override {
auto Pos = AbsolutePath.find(TestRoot);
assert(Pos != llvm::StringRef::npos);
-return FileURI::create(
+return URI::create(
Scheme, /*Authority=*/"",
AbsolutePath.substr(Pos + llvm::StringRef(TestRoot).size()));
}
@@ -57,22 +57,22 @@
std::string createOrDie(llvm::StringRef AbsolutePath,
llvm::StringRef Scheme = "file") {
- auto Uri = FileURI::create(AbsolutePath, Scheme);
+ auto Uri = URI::create(AbsolutePath, Scheme);
if (!Uri)
llvm_unreachable(llvm::toString(Uri.takeError()).c_str());
return Uri->toString();
}
std::string createOrDie(llvm::StringRef Scheme, llvm::StringRef Authority,
llvm::StringRef Body) {
- auto Uri = FileURI::create(Scheme, Authority, Body);
+ auto Uri = URI::create(Scheme, Authority, Body);
if (!Uri)
llvm_unreachable(llvm::toString(Uri.takeError()).c_str());
return Uri->toString();
}
-FileURI parseOrDie(llvm::StringRef Uri) {
- auto U = FileURI::parse(Uri);
+URI parseOrDie(llvm::StringRef Uri) {
+ auto U = URI::parse(Uri);
if (!U)
llvm_unreachable(llvm::toString(U.takeError()).c_str());
return *U;
@@ -93,8 +93,8 @@
EXPECT_EQ(parseOrDie("x:a%21b%3ac~").body(), "a!b:c~");
}
-std::string resolveOrDie(const FileURI , llvm::StringRef HintPath = "") {
- auto Path = FileURI::resolve(U, HintPath);
+std::string resolveOrDie(const URI , llvm::StringRef HintPath = "") {
+ auto Path = URI::resolve(U, HintPath);
if (!Path)
llvm_unreachable(llvm::toString(Path.takeError()).c_str());
return *Path;
@@ -110,7 +110,7 @@
}
TEST(URITest, FailedCreate) {
- auto Fail = [](llvm::Expected U) {
+ auto Fail = [](llvm::Expected U) {
if (!U) {
llvm::consumeError(U.takeError());
return true;
@@ -120,15 +120,15 @@
// Create from scheme+authority+body:
//
// Scheme must be provided.
- EXPECT_TRUE(Fail(FileURI::create("", "auth", "/a")));
+ EXPECT_TRUE(Fail(URI::create("", "auth", "/a")));
// Body must start with '/' if authority is present.
- EXPECT_TRUE(Fail(FileURI::create("scheme", "auth", "x/y/z")));
+ EXPECT_TRUE(Fail(URI::create("scheme", "auth", "x/y/z")));
// Create from scheme registry:
//
- EXPECT_TRUE(Fail(FileURI::create("/x/y/z", "no")));
+ EXPECT_TRUE(Fail(URI::create("/x/y/z", "no")));
// Path has to be absolute.
- EXPECT_TRUE(Fail(FileURI::create("x/y/z")));
+ EXPECT_TRUE(Fail(URI::create("x/y/z")));
}
TEST(URITest, Parse) {
@@ -163,7 +163,7 @@
TEST(URITest, ParseFailed) {
auto FailedParse = [](llvm::StringRef U) {
-auto URI = FileURI::parse(U);
+auto URI = URI::parse(U);
if (!URI) {
llvm::consumeError(URI.takeError());
return true;
@@ -194,14 +194,14 @@
TEST(URITest, Platform) {
auto Path = getVirtualTestFilePath("x");
- auto U = FileURI::create(Path, "file");
+ auto U = URI::create(Path, "file");
EXPECT_TRUE(static_cast(U));
EXPECT_THAT(resolveOrDie(*U), Path.str());
}
TEST(URITest, ResolveFailed) {
auto FailedResolve = [](llvm::StringRef Uri) {
-auto Path = FileURI::resolve(parseOrDie(Uri));
+auto Path = URI::resolve(parseOrDie(Uri));
if (!Path) {
llvm::consumeError(Path.takeError());
