[PATCH] D45426: [clangd] Allow using customized include path in URI.
This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE329578: [clangd] Allow using customized include path in
URI. (authored by hokein, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D45426?vs=141654&id=141655#toc
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D45426
Files:
clangd/ClangdServer.cpp
clangd/URI.cpp
clangd/URI.h
unittests/clangd/ClangdTests.cpp
unittests/clangd/TestScheme.h
Index: unittests/clangd/ClangdTests.cpp
===
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -152,6 +152,28 @@
}
};
+constexpr const char* ClangdTestScheme = "ClangdTests";
+class TestURIScheme : public URIScheme {
+public:
+ llvm::Expected
+ getAbsolutePath(llvm::StringRef /*Authority*/, llvm::StringRef Body,
+ llvm::StringRef /*HintPath*/) const override {
+llvm_unreachable("ClangdTests never makes absolute path.");
+ }
+
+ llvm::Expected
+ uriFromAbsolutePath(llvm::StringRef AbsolutePath) const override {
+llvm_unreachable("ClangdTest never creates a test URI.");
+ }
+
+ llvm::Expected getIncludeSpelling(const URI &U) const override {
+return ("\"" + U.body().trim("/") + "\"").str();
+ }
+};
+
+static URISchemeRegistry::Add
+X(ClangdTestScheme, "Test scheme for ClangdTests.");
+
TEST_F(ClangdVFSTest, Parse) {
// FIXME: figure out a stable format for AST dumps, so that we can check the
// output of the dump itself is equal to the expected one, not just that it's
@@ -961,6 +983,10 @@
/*Preferred=*/"", ""));
EXPECT_TRUE(Inserted(OriginalHeader, PreferredHeader, "\"Y.h\""));
EXPECT_TRUE(Inserted("", PreferredHeader, "\"Y.h\""));
+ auto TestURIHeader =
+ URI::parse(llvm::formatv("{0}:///x/y/z.h", ClangdTestScheme).str());
+ EXPECT_TRUE(static_cast(TestURIHeader));
+ EXPECT_TRUE(Inserted(TestURIHeader->toString(), "", "\"x/y/z.h\""));
// Check that includes are sorted.
const auto Expected = R"cpp(
Index: clangd/ClangdServer.cpp
===
--- clangd/ClangdServer.cpp
+++ clangd/ClangdServer.cpp
@@ -286,6 +286,13 @@
auto U = URI::parse(Header);
if (!U)
return U.takeError();
+
+ auto IncludePath = URI::includeSpelling(*U);
+ if (!IncludePath)
+return IncludePath.takeError();
+ if (!IncludePath->empty())
+return HeaderFile{std::move(*IncludePath), /*Verbatim=*/true};
+
auto Resolved = URI::resolve(*U, HintPath);
if (!Resolved)
return Resolved.takeError();
Index: clangd/URI.cpp
===
--- clangd/URI.cpp
+++ clangd/URI.cpp
@@ -196,5 +196,12 @@
return S->get()->getAbsolutePath(Uri.Authority, Uri.Body, HintPath);
}
+llvm::Expected URI::includeSpelling(const URI &Uri) {
+ auto S = findSchemeByName(Uri.Scheme);
+ if (!S)
+return S.takeError();
+ return S->get()->getIncludeSpelling(Uri);
+}
+
} // namespace clangd
} // namespace clang
Index: clangd/URI.h
===
--- clangd/URI.h
+++ clangd/URI.h
@@ -60,6 +60,16 @@
static llvm::Expected resolve(const URI &U,
llvm::StringRef HintPath = "");
+ /// Gets the preferred spelling of this file for #include, if there is one,
+ /// e.g. , "path/to/x.h".
+ ///
+ /// This allows URI schemas to provide their customized include paths.
+ ///
+ /// Returns an empty string if normal include-shortening based on the absolute
+ /// path should be used.
+ /// Fails if the URI is not valid in the schema.
+ static llvm::Expected includeSpelling(const URI &U);
+
friend bool operator==(const URI &LHS, const URI &RHS) {
return std::tie(LHS.Scheme, LHS.Authority, LHS.Body) ==
std::tie(RHS.Scheme, RHS.Authority, RHS.Body);
@@ -94,6 +104,13 @@
virtual llvm::Expected
uriFromAbsolutePath(llvm::StringRef AbsolutePath) const = 0;
+
+ /// Returns the include path of the file (e.g. , "path"), which can be
+ /// #included directly. See URI::includeSpelling for details.
+ virtual llvm::Expected
+ getIncludeSpelling(const URI& U) const {
+return ""; // no customized include path for this scheme.
+ }
};
/// By default, a "file" scheme is supported where URI paths are always absolute
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45426: [clangd] Allow using customized include path in URI.
hokein updated this revision to Diff 141654.
hokein marked an inline comment as done.
hokein added a comment.
includeSpelling
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D45426
Files:
clangd/ClangdServer.cpp
clangd/URI.cpp
clangd/URI.h
unittests/clangd/ClangdTests.cpp
unittests/clangd/TestScheme.h
Index: unittests/clangd/ClangdTests.cpp
===
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -152,6 +152,28 @@
}
};
+constexpr const char* ClangdTestScheme = "ClangdTests";
+class TestURIScheme : public URIScheme {
+public:
+ llvm::Expected
+ getAbsolutePath(llvm::StringRef /*Authority*/, llvm::StringRef Body,
+ llvm::StringRef /*HintPath*/) const override {
+llvm_unreachable("ClangdTests never makes absolute path.");
+ }
+
+ llvm::Expected
+ uriFromAbsolutePath(llvm::StringRef AbsolutePath) const override {
+llvm_unreachable("ClangdTest never creates a test URI.");
+ }
+
+ llvm::Expected getIncludeSpelling(const URI &U) const override {
+return ("\"" + U.body().trim("/") + "\"").str();
+ }
+};
+
+static URISchemeRegistry::Add
+X(ClangdTestScheme, "Test scheme for ClangdTests.");
+
TEST_F(ClangdVFSTest, Parse) {
// FIXME: figure out a stable format for AST dumps, so that we can check the
// output of the dump itself is equal to the expected one, not just that it's
@@ -961,6 +983,10 @@
/*Preferred=*/"", ""));
EXPECT_TRUE(Inserted(OriginalHeader, PreferredHeader, "\"Y.h\""));
EXPECT_TRUE(Inserted("", PreferredHeader, "\"Y.h\""));
+ auto TestURIHeader =
+ URI::parse(llvm::formatv("{0}:///x/y/z.h", ClangdTestScheme).str());
+ EXPECT_TRUE(static_cast(TestURIHeader));
+ EXPECT_TRUE(Inserted(TestURIHeader->toString(), "", "\"x/y/z.h\""));
// Check that includes are sorted.
const auto Expected = R"cpp(
Index: clangd/URI.h
===
--- clangd/URI.h
+++ clangd/URI.h
@@ -60,6 +60,16 @@
static llvm::Expected resolve(const URI &U,
llvm::StringRef HintPath = "");
+ /// Gets the preferred spelling of this file for #include, if there is one,
+ /// e.g. , "path/to/x.h".
+ ///
+ /// This allows URI schemas to provide their customized include paths.
+ ///
+ /// Returns an empty string if normal include-shortening based on the absolute
+ /// path should be used.
+ /// Fails if the URI is not valid in the schema.
+ static llvm::Expected includeSpelling(const URI &U);
+
friend bool operator==(const URI &LHS, const URI &RHS) {
return std::tie(LHS.Scheme, LHS.Authority, LHS.Body) ==
std::tie(RHS.Scheme, RHS.Authority, RHS.Body);
@@ -94,6 +104,13 @@
virtual llvm::Expected
uriFromAbsolutePath(llvm::StringRef AbsolutePath) const = 0;
+
+ /// Returns the include path of the file (e.g. , "path"), which can be
+ /// #included directly. See URI::includeSpelling for details.
+ virtual llvm::Expected
+ getIncludeSpelling(const URI& U) const {
+return ""; // no customized include path for this scheme.
+ }
};
/// By default, a "file" scheme is supported where URI paths are always absolute
Index: clangd/URI.cpp
===
--- clangd/URI.cpp
+++ clangd/URI.cpp
@@ -196,5 +196,12 @@
return S->get()->getAbsolutePath(Uri.Authority, Uri.Body, HintPath);
}
+llvm::Expected URI::includeSpelling(const URI &Uri) {
+ auto S = findSchemeByName(Uri.Scheme);
+ if (!S)
+return S.takeError();
+ return S->get()->getIncludeSpelling(Uri);
+}
+
} // namespace clangd
} // namespace clang
Index: clangd/ClangdServer.cpp
===
--- clangd/ClangdServer.cpp
+++ clangd/ClangdServer.cpp
@@ -286,6 +286,13 @@
auto U = URI::parse(Header);
if (!U)
return U.takeError();
+
+ auto IncludePath = URI::includeSpelling(*U);
+ if (!IncludePath)
+return IncludePath.takeError();
+ if (!IncludePath->empty())
+return HeaderFile{std::move(*IncludePath), /*Verbatim=*/true};
+
auto Resolved = URI::resolve(*U, HintPath);
if (!Resolved)
return Resolved.takeError();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45426: [clangd] Allow using customized include path in URI.
sammccall accepted this revision. sammccall added inline comments. This revision is now accepted and ready to land. Comment at: clangd/URI.h:69 + /// calculate the include path from the resolved absolute path. + static llvm::Expected includePath(const URI &U); + hokein wrote: > sammccall wrote: > > i'd avoid the name "include path" because it can be confused with a) the > > set of directories passed to -I and b) the filesystem path to the file to > > be included. > > Suggest includeString or so > Renamed it to `spellingInclude`. nit: I think `includeSpelling` would be clearer, since include is the adjective. Up to you Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D45426 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45426: [clangd] Allow using customized include path in URI.
hokein marked an inline comment as done.
hokein added inline comments.
Comment at: clangd/URI.h:66
+ ///
+ /// If the returned string is non-empty, clangd will use it directly when
+ /// doing include insertion; otherwise we will fall back to the clang to
sammccall wrote:
> does this include ? it probably should, if we're allowing schemes to
> customize how includes are spelled.
Yes, the returned string is a literal string quoted with either <> or "".
Comment at: clangd/URI.h:69
+ /// calculate the include path from the resolved absolute path.
+ static llvm::Expected includePath(const URI &U);
+
sammccall wrote:
> i'd avoid the name "include path" because it can be confused with a) the set
> of directories passed to -I and b) the filesystem path to the file to be
> included.
> Suggest includeString or so
Renamed it to `spellingInclude`.
Comment at: unittests/clangd/ClangdTests.cpp:964
EXPECT_TRUE(Inserted("", PreferredHeader, "\"Y.h\""));
+ auto TestURIHeader = URI::create("/abc/test-root/x/y/z.h", "unittest");
+ EXPECT_TRUE(static_cast(TestURIHeader));
sammccall wrote:
> this relies on ClangdTests and URITests being linked together, which we chose
> to avoid last time this came up. Just define a scheme here?
> (This is one place where putting a field on URI would have been simpler)
I think we probably want a scheme for unittest purpose (we also have one for
lit test), but this case only needs a trivial one, added here.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D45426
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45426: [clangd] Allow using customized include path in URI.
hokein updated this revision to Diff 141650.
hokein marked 3 inline comments as done.
hokein added a comment.
Address review comments.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D45426
Files:
clangd/ClangdServer.cpp
clangd/URI.cpp
clangd/URI.h
unittests/clangd/ClangdTests.cpp
unittests/clangd/TestScheme.h
Index: unittests/clangd/ClangdTests.cpp
===
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -152,6 +152,28 @@
}
};
+constexpr const char* ClangdTestScheme = "ClangdTests";
+class TestURIScheme : public URIScheme {
+public:
+ llvm::Expected
+ getAbsolutePath(llvm::StringRef /*Authority*/, llvm::StringRef Body,
+ llvm::StringRef /*HintPath*/) const override {
+llvm_unreachable("ClangdTests never makes absolute path.");
+ }
+
+ llvm::Expected
+ uriFromAbsolutePath(llvm::StringRef AbsolutePath) const override {
+llvm_unreachable("ClangdTest never creates a test URI.");
+ }
+
+ llvm::Expected getSpellingInclude(const URI &U) const override {
+return ("\"" + U.body().trim("/") + "\"").str();
+ }
+};
+
+static URISchemeRegistry::Add
+X(ClangdTestScheme, "Test scheme for ClangdTests.");
+
TEST_F(ClangdVFSTest, Parse) {
// FIXME: figure out a stable format for AST dumps, so that we can check the
// output of the dump itself is equal to the expected one, not just that it's
@@ -961,6 +983,10 @@
/*Preferred=*/"", ""));
EXPECT_TRUE(Inserted(OriginalHeader, PreferredHeader, "\"Y.h\""));
EXPECT_TRUE(Inserted("", PreferredHeader, "\"Y.h\""));
+ auto TestURIHeader =
+ URI::parse(llvm::formatv("{0}:///x/y/z.h", ClangdTestScheme).str());
+ EXPECT_TRUE(static_cast(TestURIHeader));
+ EXPECT_TRUE(Inserted(TestURIHeader->toString(), "", "\"x/y/z.h\""));
// Check that includes are sorted.
const auto Expected = R"cpp(
Index: clangd/URI.h
===
--- clangd/URI.h
+++ clangd/URI.h
@@ -60,6 +60,16 @@
static llvm::Expected resolve(const URI &U,
llvm::StringRef HintPath = "");
+ /// Gets the preferred spelling of this file for #include, if there is one,
+ /// e.g. , "path/to/x.h".
+ ///
+ /// This allows URI schemas to provide their customized include paths.
+ ///
+ /// Returns an empty string if normal include-shortening based on the absolute
+ /// path should be used.
+ /// Fails if the URI is not valid in the schema.
+ static llvm::Expected spellingInclude(const URI &U);
+
friend bool operator==(const URI &LHS, const URI &RHS) {
return std::tie(LHS.Scheme, LHS.Authority, LHS.Body) ==
std::tie(RHS.Scheme, RHS.Authority, RHS.Body);
@@ -94,6 +104,13 @@
virtual llvm::Expected
uriFromAbsolutePath(llvm::StringRef AbsolutePath) const = 0;
+
+ /// Returns the include path of the file (e.g. , "path"), which can be
+ /// #included directly. See URI::spellingInclude for details.
+ virtual llvm::Expected
+ getSpellingInclude(const URI& U) const {
+return ""; // no customized include path for this scheme.
+ }
};
/// By default, a "file" scheme is supported where URI paths are always absolute
Index: clangd/URI.cpp
===
--- clangd/URI.cpp
+++ clangd/URI.cpp
@@ -196,5 +196,12 @@
return S->get()->getAbsolutePath(Uri.Authority, Uri.Body, HintPath);
}
+llvm::Expected URI::spellingInclude(const URI &Uri) {
+ auto S = findSchemeByName(Uri.Scheme);
+ if (!S)
+return S.takeError();
+ return S->get()->getSpellingInclude(Uri);
+}
+
} // namespace clangd
} // namespace clang
Index: clangd/ClangdServer.cpp
===
--- clangd/ClangdServer.cpp
+++ clangd/ClangdServer.cpp
@@ -286,6 +286,13 @@
auto U = URI::parse(Header);
if (!U)
return U.takeError();
+
+ auto IncludePath = URI::spellingInclude(*U);
+ if (!IncludePath)
+return IncludePath.takeError();
+ if (!IncludePath->empty())
+return HeaderFile{std::move(*IncludePath), /*Verbatim=*/true};
+
auto Resolved = URI::resolve(*U, HintPath);
if (!Resolved)
return Resolved.takeError();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45426: [clangd] Allow using customized include path in URI.
sammccall added inline comments.
Comment at: clangd/URI.h:63
+ /// Tries to get the include path of the file corresponding to the URI.
+ /// This allows clients to provide their customized include paths.
Avoid "tries to" which is vague about what cases are failures.
e.g.
Get the preferred spelling of this file for #include, if there is one, e.g.
.
Returns the empty string if normal include-shortening based on the path should
be used.
Fails if the URI is not valid in the schema.
Comment at: clangd/URI.h:64
+ /// Tries to get the include path of the file corresponding to the URI.
+ /// This allows clients to provide their customized include paths.
+ ///
what are "clients"? maybe "URI schemas"?
Comment at: clangd/URI.h:66
+ ///
+ /// If the returned string is non-empty, clangd will use it directly when
+ /// doing include insertion; otherwise we will fall back to the clang to
does this include ? it probably should, if we're allowing schemes to
customize how includes are spelled.
Comment at: clangd/URI.h:69
+ /// calculate the include path from the resolved absolute path.
+ static llvm::Expected includePath(const URI &U);
+
i'd avoid the name "include path" because it can be confused with a) the set of
directories passed to -I and b) the filesystem path to the file to be included.
Suggest includeString or so
Comment at: clangd/URI.h:107
+ /// Returns the include path of the file corresponding to the URI, which can
+ /// be #include directly. See URI::resolveIncludePath for details.
+ virtual llvm::Expected
#included
Comment at: unittests/clangd/ClangdTests.cpp:964
EXPECT_TRUE(Inserted("", PreferredHeader, "\"Y.h\""));
+ auto TestURIHeader = URI::create("/abc/test-root/x/y/z.h", "unittest");
+ EXPECT_TRUE(static_cast(TestURIHeader));
this relies on ClangdTests and URITests being linked together, which we chose
to avoid last time this came up. Just define a scheme here?
(This is one place where putting a field on URI would have been simpler)
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D45426
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45426: [clangd] Allow using customized include path in URI.
hokein created this revision.
hokein added a reviewer: sammccall.
Herald added subscribers: MaskRay, ioeric, jkorous-apple, ilya-biryukov, klimek.
Calculating the include path from absolute file path does not always
work for all build system, e.g. bazel uses symlink as the build working
directory. The absolute file path from editor and clang is diverged from
each other. We need to address it properly in build sysmtem integration.
This patch worksarounds the issue by providing a hook in URI which allows
clients to provide their customized include path.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D45426
Files:
clangd/ClangdServer.cpp
clangd/URI.cpp
clangd/URI.h
unittests/clangd/ClangdTests.cpp
unittests/clangd/TestScheme.h
unittests/clangd/URITests.cpp
Index: unittests/clangd/URITests.cpp
===
--- unittests/clangd/URITests.cpp
+++ unittests/clangd/URITests.cpp
@@ -47,6 +47,10 @@
return URI(Scheme, /*Authority=*/"",
AbsolutePath.substr(Pos + llvm::StringRef(TestRoot).size()));
}
+
+ llvm::Expected getIncludePath(const URI &U) const override {
+return ("\"" + U.body() + "\"").str();
+ }
};
const char *TestScheme::Scheme = "unittest";
Index: unittests/clangd/ClangdTests.cpp
===
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -961,6 +961,10 @@
/*Preferred=*/"", ""));
EXPECT_TRUE(Inserted(OriginalHeader, PreferredHeader, "\"Y.h\""));
EXPECT_TRUE(Inserted("", PreferredHeader, "\"Y.h\""));
+ auto TestURIHeader = URI::create("/abc/test-root/x/y/z.h", "unittest");
+ EXPECT_TRUE(static_cast(TestURIHeader));
+ EXPECT_TRUE(Inserted(TestURIHeader->toString(), "", "\"x/y/z.h\""));
+
// Check that includes are sorted.
const auto Expected = R"cpp(
Index: clangd/URI.h
===
--- clangd/URI.h
+++ clangd/URI.h
@@ -60,6 +60,14 @@
static llvm::Expected resolve(const URI &U,
llvm::StringRef HintPath = "");
+ /// Tries to get the include path of the file corresponding to the URI.
+ /// This allows clients to provide their customized include paths.
+ ///
+ /// If the returned string is non-empty, clangd will use it directly when
+ /// doing include insertion; otherwise we will fall back to the clang to
+ /// calculate the include path from the resolved absolute path.
+ static llvm::Expected includePath(const URI &U);
+
friend bool operator==(const URI &LHS, const URI &RHS) {
return std::tie(LHS.Scheme, LHS.Authority, LHS.Body) ==
std::tie(RHS.Scheme, RHS.Authority, RHS.Body);
@@ -94,6 +102,13 @@
virtual llvm::Expected
uriFromAbsolutePath(llvm::StringRef AbsolutePath) const = 0;
+
+ /// Returns the include path of the file corresponding to the URI, which can
+ /// be #include directly. See URI::resolveIncludePath for details.
+ virtual llvm::Expected
+ getIncludePath(const URI& U) const {
+return ""; // no customized include path for this scheme.
+ }
};
/// By default, a "file" scheme is supported where URI paths are always
absolute
Index: clangd/URI.cpp
===
--- clangd/URI.cpp
+++ clangd/URI.cpp
@@ -196,5 +196,12 @@
return S->get()->getAbsolutePath(Uri.Authority, Uri.Body, HintPath);
}
+llvm::Expected URI::includePath(const URI &Uri) {
+ auto S = findSchemeByName(Uri.Scheme);
+ if (!S)
+return S.takeError();
+ return S->get()->getIncludePath(Uri);
+}
+
} // namespace clangd
} // namespace clang
Index: clangd/ClangdServer.cpp
===
--- clangd/ClangdServer.cpp
+++ clangd/ClangdServer.cpp
@@ -286,6 +286,13 @@
auto U = URI::parse(Header);
if (!U)
return U.takeError();
+
+ auto IncludePath = URI::includePath(*U);
+ if (!IncludePath)
+return IncludePath.takeError();
+ if (!IncludePath->empty())
+return HeaderFile{std::move(*IncludePath), /*Verbatim=*/true};
+
auto Resolved = URI::resolve(*U, HintPath);
if (!Resolved)
return Resolved.takeError();
Index: unittests/clangd/URITests.cpp
===
--- unittests/clangd/URITests.cpp
+++ unittests/clangd/URITests.cpp
@@ -47,6 +47,10 @@
return URI(Scheme, /*Authority=*/"",
AbsolutePath.substr(Pos + llvm::StringRef(TestRoot).size()));
}
+
+ llvm::Expected getIncludePath(const URI &U) const override {
+return ("\"" + U.body() + "\"").str();
+ }
};
const char *TestScheme::Scheme = "unittest";
Index: unittests/clangd/ClangdTests.cpp
===
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -961,6 +961,10 @@
