[PATCH] D42915: [clangd] Use URIs in index symbols.
This revision was automatically updated to reflect the committed changes.
Closed by commit rL324358: [clangd] Use URIs in index symbols. (authored by
ioeric, committed by ).
Herald added a subscriber: llvm-commits.
Repository:
rL LLVM
https://reviews.llvm.org/D42915
Files:
clang-tools-extra/trunk/clangd/index/Index.cpp
clang-tools-extra/trunk/clangd/index/Index.h
clang-tools-extra/trunk/clangd/index/Merge.cpp
clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp
clang-tools-extra/trunk/clangd/index/SymbolCollector.h
clang-tools-extra/trunk/clangd/index/SymbolYAML.cpp
clang-tools-extra/trunk/unittests/clangd/IndexTests.cpp
clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp
Index: clang-tools-extra/trunk/unittests/clangd/IndexTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/IndexTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/IndexTests.cpp
@@ -226,8 +226,8 @@
Symbol L, R;
L.ID = R.ID = SymbolID("hello");
L.Name = R.Name = "Foo";// same in both
- L.CanonicalDeclaration.FilePath = "left.h"; // differs
- R.CanonicalDeclaration.FilePath = "right.h";
+ L.CanonicalDeclaration.FileURI = "file:///left.h"; // differs
+ R.CanonicalDeclaration.FileURI = "file:///right.h";
L.CompletionPlainInsertText = "f00";// present in left only
R.CompletionSnippetInsertText = "f0{$1:0}"; // present in right only
Symbol::Details DetL, DetR;
@@ -240,7 +240,7 @@
Symbol::Details Scratch;
Symbol M = mergeSymbol(L, R, &Scratch);
EXPECT_EQ(M.Name, "Foo");
- EXPECT_EQ(M.CanonicalDeclaration.FilePath, "left.h");
+ EXPECT_EQ(M.CanonicalDeclaration.FileURI, "file:///left.h");
EXPECT_EQ(M.CompletionPlainInsertText, "f00");
EXPECT_EQ(M.CompletionSnippetInsertText, "f0{$1:0}");
ASSERT_TRUE(M.Detail);
Index: clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp
@@ -46,7 +46,7 @@
return arg.CompletionSnippetInsertText == S;
}
MATCHER_P(QName, Name, "") { return (arg.Scope + arg.Name).str() == Name; }
-MATCHER_P(CPath, P, "") { return arg.CanonicalDeclaration.FilePath == P; }
+MATCHER_P(DeclURI, P, "") { return arg.CanonicalDeclaration.FileURI == P; }
MATCHER_P(LocationOffsets, Offsets, "") {
// Offset range in SymbolLocation is [start, end] while in Clangd is [start,
// end).
@@ -58,8 +58,6 @@
namespace clangd {
namespace {
-const char TestHeaderName[] = "symbols.h";
-const char TestFileName[] = "symbol.cc";
class SymbolIndexActionFactory : public tooling::FrontendActionFactory {
public:
SymbolIndexActionFactory(SymbolCollector::Options COpts)
@@ -82,6 +80,13 @@
class SymbolCollectorTest : public ::testing::Test {
public:
+ SymbolCollectorTest()
+ : TestHeaderName(getVirtualTestFilePath("symbol.h").str()),
+TestFileName(getVirtualTestFilePath("symbol.cc").str()) {
+TestHeaderURI = URI::createFile(TestHeaderName).toString();
+TestFileURI = URI::createFile(TestFileName).toString();
+ }
+
bool runSymbolCollector(StringRef HeaderCode, StringRef MainCode,
const std::vector &ExtraArgs = {}) {
llvm::IntrusiveRefCntPtr InMemoryFileSystem(
@@ -104,15 +109,21 @@
std::string Content = MainCode;
if (!HeaderCode.empty())
- Content = "#include\"" + std::string(TestHeaderName) + "\"\n" + Content;
+ Content = (llvm::Twine("#include\"") +
+ llvm::sys::path::filename(TestHeaderName) + "\"\n" + Content)
+.str();
InMemoryFileSystem->addFile(TestFileName, 0,
llvm::MemoryBuffer::getMemBuffer(Content));
Invocation.run();
Symbols = Factory->Collector->takeSymbols();
return true;
}
protected:
+ std::string TestHeaderName;
+ std::string TestHeaderURI;
+ std::string TestFileName;
+ std::string TestFileURI;
SymbolSlab Symbols;
SymbolCollector::Options CollectorOpts;
};
@@ -169,16 +180,49 @@
CollectorOpts.IndexMainFiles = false;
runSymbolCollector("class Foo {};", /*Main=*/"");
EXPECT_THAT(Symbols,
- UnorderedElementsAre(AllOf(QName("Foo"), CPath("symbols.h";
+ UnorderedElementsAre(AllOf(QName("Foo"), DeclURI(TestHeaderURI;
}
TEST_F(SymbolCollectorTest, SymbolRelativeWithFallback) {
CollectorOpts.IndexMainFiles = false;
+ TestHeaderName = "x.h";
+ TestFileName = "x.cpp";
+ TestHeaderURI =
+ URI::createFile(getVirtualTestFilePath(TestHeaderName)).toString();
CollectorOpts.FallbackDir = getVirtualTestRoot();
runSymbolCollector("class Foo {};", /*Main=*/"");
EXPECT_THAT(Symbols,
- UnorderedElementsAre(AllOf(
- QName("Foo"), CPath(getVirtualTestFilePath("symbols.h");
+
[PATCH] D42915: [clangd] Use URIs in index symbols.
ioeric updated this revision to Diff 133009.
ioeric added a comment.
- removed leftover logs.
- Merge branch 'master' of http://llvm.org/git/clang-tools-extra into uri
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42915
Files:
clangd/index/Index.cpp
clangd/index/Index.h
clangd/index/Merge.cpp
clangd/index/SymbolCollector.cpp
clangd/index/SymbolCollector.h
clangd/index/SymbolYAML.cpp
unittests/clangd/IndexTests.cpp
unittests/clangd/SymbolCollectorTests.cpp
Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -46,7 +46,7 @@
return arg.CompletionSnippetInsertText == S;
}
MATCHER_P(QName, Name, "") { return (arg.Scope + arg.Name).str() == Name; }
-MATCHER_P(CPath, P, "") { return arg.CanonicalDeclaration.FilePath == P; }
+MATCHER_P(DeclURI, P, "") { return arg.CanonicalDeclaration.FileURI == P; }
MATCHER_P(LocationOffsets, Offsets, "") {
// Offset range in SymbolLocation is [start, end] while in Clangd is [start,
// end).
@@ -58,8 +58,6 @@
namespace clangd {
namespace {
-const char TestHeaderName[] = "symbols.h";
-const char TestFileName[] = "symbol.cc";
class SymbolIndexActionFactory : public tooling::FrontendActionFactory {
public:
SymbolIndexActionFactory(SymbolCollector::Options COpts)
@@ -82,6 +80,13 @@
class SymbolCollectorTest : public ::testing::Test {
public:
+ SymbolCollectorTest()
+ : TestHeaderName(getVirtualTestFilePath("symbol.h").str()),
+TestFileName(getVirtualTestFilePath("symbol.cc").str()) {
+TestHeaderURI = URI::createFile(TestHeaderName).toString();
+TestFileURI = URI::createFile(TestFileName).toString();
+ }
+
bool runSymbolCollector(StringRef HeaderCode, StringRef MainCode,
const std::vector &ExtraArgs = {}) {
llvm::IntrusiveRefCntPtr InMemoryFileSystem(
@@ -104,15 +109,21 @@
std::string Content = MainCode;
if (!HeaderCode.empty())
- Content = "#include\"" + std::string(TestHeaderName) + "\"\n" + Content;
+ Content = (llvm::Twine("#include\"") +
+ llvm::sys::path::filename(TestHeaderName) + "\"\n" + Content)
+.str();
InMemoryFileSystem->addFile(TestFileName, 0,
llvm::MemoryBuffer::getMemBuffer(Content));
Invocation.run();
Symbols = Factory->Collector->takeSymbols();
return true;
}
protected:
+ std::string TestHeaderName;
+ std::string TestHeaderURI;
+ std::string TestFileName;
+ std::string TestFileURI;
SymbolSlab Symbols;
SymbolCollector::Options CollectorOpts;
};
@@ -169,16 +180,49 @@
CollectorOpts.IndexMainFiles = false;
runSymbolCollector("class Foo {};", /*Main=*/"");
EXPECT_THAT(Symbols,
- UnorderedElementsAre(AllOf(QName("Foo"), CPath("symbols.h";
+ UnorderedElementsAre(AllOf(QName("Foo"), DeclURI(TestHeaderURI;
}
TEST_F(SymbolCollectorTest, SymbolRelativeWithFallback) {
CollectorOpts.IndexMainFiles = false;
+ TestHeaderName = "x.h";
+ TestFileName = "x.cpp";
+ TestHeaderURI =
+ URI::createFile(getVirtualTestFilePath(TestHeaderName)).toString();
CollectorOpts.FallbackDir = getVirtualTestRoot();
runSymbolCollector("class Foo {};", /*Main=*/"");
EXPECT_THAT(Symbols,
- UnorderedElementsAre(AllOf(
- QName("Foo"), CPath(getVirtualTestFilePath("symbols.h");
+ UnorderedElementsAre(AllOf(QName("Foo"), DeclURI(TestHeaderURI;
+}
+
+#ifndef LLVM_ON_WIN32
+TEST_F(SymbolCollectorTest, CustomURIScheme) {
+ CollectorOpts.IndexMainFiles = false;
+ // Use test URI scheme from URITests.cpp
+ CollectorOpts.URISchemes.insert(CollectorOpts.URISchemes.begin(), "unittest");
+ TestHeaderName = getVirtualTestFilePath("test-root/x.h").str();
+ TestFileName = getVirtualTestFilePath("test-root/x.cpp").str();
+ runSymbolCollector("class Foo {};", /*Main=*/"");
+ EXPECT_THAT(Symbols,
+ UnorderedElementsAre(AllOf(QName("Foo"), DeclURI("unittest:x.h";
+}
+#endif
+
+TEST_F(SymbolCollectorTest, InvalidURIScheme) {
+ CollectorOpts.IndexMainFiles = false;
+ // Use test URI scheme from URITests.cpp
+ CollectorOpts.URISchemes = {"invalid"};
+ runSymbolCollector("class Foo {};", /*Main=*/"");
+ EXPECT_THAT(Symbols, UnorderedElementsAre(AllOf(QName("Foo"), DeclURI("";
+}
+
+TEST_F(SymbolCollectorTest, FallbackToFileURI) {
+ CollectorOpts.IndexMainFiles = false;
+ // Use test URI scheme from URITests.cpp
+ CollectorOpts.URISchemes = {"invalid", "file"};
+ runSymbolCollector("class Foo {};", /*Main=*/"");
+ EXPECT_THAT(Symbols, UnorderedElementsAre(
+ AllOf(QName("Foo"), DeclURI(TestHeaderURI;
}
TEST_F(SymbolCollectorTest, IncludeEnums) {
@@ -233,14 +277,14 @@
)");
runSymbolCollector(Header.code(), /*Main=*/"");
- EXPECT_THAT
[PATCH] D42915: [clangd] Use URIs in index symbols.
sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.
Comment at: unittests/clangd/SymbolCollectorTests.cpp:280
runSymbolCollector(Header.code(), /*Main=*/"");
- EXPECT_THAT(Symbols,
- UnorderedElementsAre(
- AllOf(QName("abc_Test"),
-LocationOffsets(Header.offsetRange("expansion")),
-CPath(TestHeaderName)),
- AllOf(QName("Test"),
-LocationOffsets(Header.offsetRange("spelling")),
-CPath(TestHeaderName;
+ for (const auto &S : Symbols)
+llvm::errs() << "~~~ " << S.CanonicalDeclaration.FileURI << ": "
leftover logs
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42915
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42915: [clangd] Use URIs in index symbols.
ioeric updated this revision to Diff 133000.
ioeric added a comment.
- s/Uri/URI/
- Address review comments. Support multiple schemes.
- Merged with origin/master
- Merge branch 'master' of http://llvm.org/git/clang-tools-extra into uri
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42915
Files:
clangd/index/Index.cpp
clangd/index/Index.h
clangd/index/Merge.cpp
clangd/index/SymbolCollector.cpp
clangd/index/SymbolCollector.h
clangd/index/SymbolYAML.cpp
unittests/clangd/IndexTests.cpp
unittests/clangd/SymbolCollectorTests.cpp
Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -46,7 +46,7 @@
return arg.CompletionSnippetInsertText == S;
}
MATCHER_P(QName, Name, "") { return (arg.Scope + arg.Name).str() == Name; }
-MATCHER_P(CPath, P, "") { return arg.CanonicalDeclaration.FilePath == P; }
+MATCHER_P(DeclURI, P, "") { return arg.CanonicalDeclaration.FileURI == P; }
MATCHER_P(LocationOffsets, Offsets, "") {
// Offset range in SymbolLocation is [start, end] while in Clangd is [start,
// end).
@@ -58,8 +58,6 @@
namespace clangd {
namespace {
-const char TestHeaderName[] = "symbols.h";
-const char TestFileName[] = "symbol.cc";
class SymbolIndexActionFactory : public tooling::FrontendActionFactory {
public:
SymbolIndexActionFactory(SymbolCollector::Options COpts)
@@ -82,6 +80,13 @@
class SymbolCollectorTest : public ::testing::Test {
public:
+ SymbolCollectorTest()
+ : TestHeaderName(getVirtualTestFilePath("symbol.h").str()),
+TestFileName(getVirtualTestFilePath("symbol.cc").str()) {
+TestHeaderURI = URI::createFile(TestHeaderName).toString();
+TestFileURI = URI::createFile(TestFileName).toString();
+ }
+
bool runSymbolCollector(StringRef HeaderCode, StringRef MainCode,
const std::vector &ExtraArgs = {}) {
llvm::IntrusiveRefCntPtr InMemoryFileSystem(
@@ -104,15 +109,21 @@
std::string Content = MainCode;
if (!HeaderCode.empty())
- Content = "#include\"" + std::string(TestHeaderName) + "\"\n" + Content;
+ Content = (llvm::Twine("#include\"") +
+ llvm::sys::path::filename(TestHeaderName) + "\"\n" + Content)
+.str();
InMemoryFileSystem->addFile(TestFileName, 0,
llvm::MemoryBuffer::getMemBuffer(Content));
Invocation.run();
Symbols = Factory->Collector->takeSymbols();
return true;
}
protected:
+ std::string TestHeaderName;
+ std::string TestHeaderURI;
+ std::string TestFileName;
+ std::string TestFileURI;
SymbolSlab Symbols;
SymbolCollector::Options CollectorOpts;
};
@@ -169,16 +180,49 @@
CollectorOpts.IndexMainFiles = false;
runSymbolCollector("class Foo {};", /*Main=*/"");
EXPECT_THAT(Symbols,
- UnorderedElementsAre(AllOf(QName("Foo"), CPath("symbols.h";
+ UnorderedElementsAre(AllOf(QName("Foo"), DeclURI(TestHeaderURI;
}
TEST_F(SymbolCollectorTest, SymbolRelativeWithFallback) {
CollectorOpts.IndexMainFiles = false;
+ TestHeaderName = "x.h";
+ TestFileName = "x.cpp";
+ TestHeaderURI =
+ URI::createFile(getVirtualTestFilePath(TestHeaderName)).toString();
CollectorOpts.FallbackDir = getVirtualTestRoot();
runSymbolCollector("class Foo {};", /*Main=*/"");
EXPECT_THAT(Symbols,
- UnorderedElementsAre(AllOf(
- QName("Foo"), CPath(getVirtualTestFilePath("symbols.h");
+ UnorderedElementsAre(AllOf(QName("Foo"), DeclURI(TestHeaderURI;
+}
+
+#ifndef LLVM_ON_WIN32
+TEST_F(SymbolCollectorTest, CustomURIScheme) {
+ CollectorOpts.IndexMainFiles = false;
+ // Use test URI scheme from URITests.cpp
+ CollectorOpts.URISchemes.insert(CollectorOpts.URISchemes.begin(), "unittest");
+ TestHeaderName = getVirtualTestFilePath("test-root/x.h").str();
+ TestFileName = getVirtualTestFilePath("test-root/x.cpp").str();
+ runSymbolCollector("class Foo {};", /*Main=*/"");
+ EXPECT_THAT(Symbols,
+ UnorderedElementsAre(AllOf(QName("Foo"), DeclURI("unittest:x.h";
+}
+#endif
+
+TEST_F(SymbolCollectorTest, InvalidURIScheme) {
+ CollectorOpts.IndexMainFiles = false;
+ // Use test URI scheme from URITests.cpp
+ CollectorOpts.URISchemes = {"invalid"};
+ runSymbolCollector("class Foo {};", /*Main=*/"");
+ EXPECT_THAT(Symbols, UnorderedElementsAre(AllOf(QName("Foo"), DeclURI("";
+}
+
+TEST_F(SymbolCollectorTest, FallbackToFileURI) {
+ CollectorOpts.IndexMainFiles = false;
+ // Use test URI scheme from URITests.cpp
+ CollectorOpts.URISchemes = {"invalid", "file"};
+ runSymbolCollector("class Foo {};", /*Main=*/"");
+ EXPECT_THAT(Symbols, UnorderedElementsAre(
+ AllOf(QName("Foo"), DeclURI(TestHeaderURI;
}
TEST_F(SymbolCollectorTest, IncludeEnums) {
@@ -233,14 +277,18 @@
)")
[PATCH] D42915: [clangd] Use URIs in index symbols.
ioeric added inline comments. Comment at: clangd/index/Index.h:27 + // The URI of the source file where a symbol occurs. + llvm::StringRef FileUri; // The 0-based offset to the first character of the symbol from the beginning hokein wrote: > sammccall wrote: > > ioeric wrote: > > > sammccall wrote: > > > > nit: FileURI? > > > > (The other style is OK too, though I personally find it harder to read. > > > > But the class is `URI` and we should be consistent) > > > I tend to use `Uri` for URI strings and `URI`/`U` for URI objects. But > > > I'd be okay with either, if you prefer `URI`. > > I'm fine with either spelling, but not really with mixing the two. > > > > Is there some precedent for using different capitalization depending on the > > variable type? That seems... non obvious to me. > I'm +1 on `URI`, using all capital letters for initialisms can make the name > a bit easier to read. Let's be consistent using `URI`? I'll change to `URI`. Another reason why I avoided using URI as variable names was the name conflicts with the actual URI class... I always need to come up with a prefix... Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D42915 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42915: [clangd] Use URIs in index symbols.
hokein added inline comments. Comment at: clangd/index/Index.h:27 + // The URI of the source file where a symbol occurs. + llvm::StringRef FileUri; // The 0-based offset to the first character of the symbol from the beginning sammccall wrote: > ioeric wrote: > > sammccall wrote: > > > nit: FileURI? > > > (The other style is OK too, though I personally find it harder to read. > > > But the class is `URI` and we should be consistent) > > I tend to use `Uri` for URI strings and `URI`/`U` for URI objects. But I'd > > be okay with either, if you prefer `URI`. > I'm fine with either spelling, but not really with mixing the two. > > Is there some precedent for using different capitalization depending on the > variable type? That seems... non obvious to me. I'm +1 on `URI`, using all capital letters for initialisms can make the name a bit easier to read. Let's be consistent using `URI`? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D42915 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42915: [clangd] Use URIs in index symbols.
sammccall added a comment.
Great, this all makes sense. I think we can/should make the scheme selection a
bit more robust (we shouldn't crash if we get unexpected filenames).
And... Uri or URI (I really think this is a usability issue - i had a scarring
experience with a codebase that couldn't decide how to spell abbreviations...)
Comment at: clangd/index/Index.h:27
+ // The URI of the source file where a symbol occurs.
+ llvm::StringRef FileUri;
// The 0-based offset to the first character of the symbol from the beginning
ioeric wrote:
> sammccall wrote:
> > nit: FileURI?
> > (The other style is OK too, though I personally find it harder to read. But
> > the class is `URI` and we should be consistent)
> I tend to use `Uri` for URI strings and `URI`/`U` for URI objects. But I'd
> be okay with either, if you prefer `URI`.
I'm fine with either spelling, but not really with mixing the two.
Is there some precedent for using different capitalization depending on the
variable type? That seems... non obvious to me.
Comment at: clangd/index/SymbolCollector.cpp:67
+ if (!U)
+llvm_unreachable(llvm::toString(U.takeError()).c_str());
+ return U->toString();
this doesn't seem unreachable?
Comment at: clangd/index/SymbolCollector.h:38
std::string FallbackDir;
+/// Specifies the URI scheme used to encode file paths in symbols.
+std::string FileURIScheme = "file";
We should document the semantics/exceptions in case of failure.
Do we drop the symbol, or drop the location, or fall back to file URI?
One scheme that would be flexible and I think pretty simple:
- make this a vector of schemes that will be tried in order
- drop the location (but not symbol) if it's not possible to encode the path
- note that the vector should probably end in "file" as a fallback, and make
`{"file"}` the default
For our monorepo we can set this to {"google", "file"} or just {"google"} if we
never want to leak local paths. It seems pretty plausible that other orgs whose
builds aren't totally hermetic would want several schemes here.
Comment at: unittests/clangd/SymbolCollectorTests.cpp:49
MATCHER_P(QName, Name, "") { return (arg.Scope + arg.Name).str() == Name; }
-MATCHER_P(CPath, P, "") { return arg.CanonicalDeclaration.FilePath == P; }
+MATCHER_P(Uri, P, "") { return arg.CanonicalDeclaration.FileUri == P; }
MATCHER_P(LocationOffsets, Offsets, "") {
while renaming this, maybe Decl is better? we'll have Def soon!
Comment at: unittests/clangd/SymbolCollectorTests.cpp:195
+#ifndef LLVM_ON_WIN32
+TEST_F(SymbolCollectorTest, CustomURIScheme) {
+ CollectorOpts.IndexMainFiles = false;
Can you add a test for the case when the path fails to convert to the URI
scheme?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42915
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42915: [clangd] Use URIs in index symbols.
ioeric added a comment. I was thinking about leaving URI scheme customization to the postprocessing phase, but you are right, it would be better to make the URI scheme extendable in SymbolCollector. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D42915 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42915: [clangd] Use URIs in index symbols.
ioeric updated this revision to Diff 132958.
ioeric marked 2 inline comments as done.
ioeric added a comment.
- Make URIScheme customizable in SymbolCollector.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42915
Files:
clangd/index/Index.cpp
clangd/index/Index.h
clangd/index/Merge.cpp
clangd/index/SymbolCollector.cpp
clangd/index/SymbolCollector.h
clangd/index/SymbolYAML.cpp
unittests/clangd/IndexTests.cpp
unittests/clangd/SymbolCollectorTests.cpp
Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -46,7 +46,7 @@
return arg.CompletionSnippetInsertText == S;
}
MATCHER_P(QName, Name, "") { return (arg.Scope + arg.Name).str() == Name; }
-MATCHER_P(CPath, P, "") { return arg.CanonicalDeclaration.FilePath == P; }
+MATCHER_P(Uri, P, "") { return arg.CanonicalDeclaration.FileUri == P; }
MATCHER_P(LocationOffsets, Offsets, "") {
// Offset range in SymbolLocation is [start, end] while in Clangd is [start,
// end).
@@ -58,8 +58,6 @@
namespace clangd {
namespace {
-const char TestHeaderName[] = "symbols.h";
-const char TestFileName[] = "symbol.cc";
class SymbolIndexActionFactory : public tooling::FrontendActionFactory {
public:
SymbolIndexActionFactory(SymbolCollector::Options COpts)
@@ -82,6 +80,13 @@
class SymbolCollectorTest : public ::testing::Test {
public:
+ SymbolCollectorTest()
+ : TestHeaderName(getVirtualTestFilePath("symbol.h").str()),
+TestFileName(getVirtualTestFilePath("symbol.cc").str()) {
+TestHeaderUri = URI::createFile(TestHeaderName).toString();
+TestFileUri = URI::createFile(TestFileName).toString();
+ }
+
bool runSymbolCollector(StringRef HeaderCode, StringRef MainCode) {
llvm::IntrusiveRefCntPtr InMemoryFileSystem(
new vfs::InMemoryFileSystem);
@@ -100,15 +105,21 @@
std::string Content = MainCode;
if (!HeaderCode.empty())
- Content = "#include\"" + std::string(TestHeaderName) + "\"\n" + Content;
+ Content = (llvm::Twine("#include\"") +
+ llvm::sys::path::filename(TestHeaderName) + "\"\n" + Content)
+.str();
InMemoryFileSystem->addFile(TestFileName, 0,
llvm::MemoryBuffer::getMemBuffer(Content));
Invocation.run();
Symbols = Factory->Collector->takeSymbols();
return true;
}
protected:
+ std::string TestHeaderName;
+ std::string TestHeaderUri;
+ std::string TestFileName;
+ std::string TestFileUri;
SymbolSlab Symbols;
SymbolCollector::Options CollectorOpts;
};
@@ -165,17 +176,33 @@
CollectorOpts.IndexMainFiles = false;
runSymbolCollector("class Foo {};", /*Main=*/"");
EXPECT_THAT(Symbols,
- UnorderedElementsAre(AllOf(QName("Foo"), CPath("symbols.h";
+ UnorderedElementsAre(AllOf(QName("Foo"), Uri(TestHeaderUri;
}
TEST_F(SymbolCollectorTest, SymbolRelativeWithFallback) {
CollectorOpts.IndexMainFiles = false;
+ TestHeaderName = "x.h";
+ TestFileName = "x.cpp";
+ TestHeaderUri =
+ URI::createFile(getVirtualTestFilePath(TestHeaderName)).toString();
CollectorOpts.FallbackDir = getVirtualTestRoot();
runSymbolCollector("class Foo {};", /*Main=*/"");
EXPECT_THAT(Symbols,
- UnorderedElementsAre(AllOf(
- QName("Foo"), CPath(getVirtualTestFilePath("symbols.h");
+ UnorderedElementsAre(AllOf(QName("Foo"), Uri(TestHeaderUri;
+}
+
+#ifndef LLVM_ON_WIN32
+TEST_F(SymbolCollectorTest, CustomURIScheme) {
+ CollectorOpts.IndexMainFiles = false;
+ // Use test URI scheme from URITests.cpp
+ CollectorOpts.FileURIScheme = "unittest";
+ TestHeaderName = getVirtualTestFilePath("test-root/x.h").str();
+ TestFileName = getVirtualTestFilePath("test-root/x.cpp").str();
+ runSymbolCollector("class Foo {};", /*Main=*/"");
+ EXPECT_THAT(Symbols,
+ UnorderedElementsAre(AllOf(QName("Foo"), Uri("unittest:x.h";
}
+#endif
TEST_F(SymbolCollectorTest, IncludeEnums) {
CollectorOpts.IndexMainFiles = false;
@@ -229,14 +256,14 @@
)");
runSymbolCollector(Header.code(), /*Main=*/"");
- EXPECT_THAT(Symbols,
- UnorderedElementsAre(
- AllOf(QName("abc_Test"),
-LocationOffsets(Header.offsetRange("expansion")),
-CPath(TestHeaderName)),
- AllOf(QName("Test"),
-LocationOffsets(Header.offsetRange("spelling")),
-CPath(TestHeaderName;
+ EXPECT_THAT(
+ Symbols,
+ UnorderedElementsAre(
+ AllOf(QName("abc_Test"),
+LocationOffsets(Header.offsetRange("expansion")),
+Uri(TestHeaderUri)),
+ AllOf(QName("Test"), LocationOffsets(Header.offsetRange("spelling")),
+Uri(TestHeaderUri;
}
TEST_F(SymbolCo
[PATCH] D42915: [clangd] Use URIs in index symbols.
ioeric added inline comments. Comment at: clangd/index/Index.h:27 + // The URI of the source file where a symbol occurs. + llvm::StringRef FileUri; // The 0-based offset to the first character of the symbol from the beginning sammccall wrote: > nit: FileURI? > (The other style is OK too, though I personally find it harder to read. But > the class is `URI` and we should be consistent) I tend to use `Uri` for URI strings and `URI`/`U` for URI objects. But I'd be okay with either, if you prefer `URI`. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D42915 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42915: [clangd] Use URIs in index symbols.
sammccall added a comment.
This looks OK so far, where is it going? It doesn't make sense to put URIs in
if we only ever use `file:`.
I guess others will be produced by some kind of extension point on
SymbolCollector. That will specify URI schemes we should try, allow you to
replace the whole `toFileURI`, or something else?
Unfortunately there's a bunch of `Uri`s here, where the existing code uses
`URI`...
Comment at: clangd/index/Index.h:27
+ // The URI of the source file where a symbol occurs.
+ llvm::StringRef FileUri;
// The 0-based offset to the first character of the symbol from the beginning
nit: FileURI?
(The other style is OK too, though I personally find it harder to read. But the
class is `URI` and we should be consistent)
Comment at: clangd/index/SymbolCollector.cpp:28
+// current working directory of the given SourceManager if the Path is not an
+// absolute path. If failed, this combine relative paths with \p FallbackDir to
+// get an absolute path.
this combines
or better, "resolves relative paths against \p FallbackDir"
Comment at: clangd/index/SymbolCollector.cpp:33
// the SourceManager.
-std::string makeAbsolutePath(const SourceManager &SM, StringRef Path,
- StringRef FallbackDir) {
+std::string toFileUri(const SourceManager &SM, StringRef Path,
+ StringRef FallbackDir) {
also URI here, and below
Comment at: clangd/index/SymbolCollector.cpp:201
+std::string Uri;
+S.CanonicalDeclaration = GetSymbolLocation(ND, SM, Opts.FallbackDir, Uri);
while here, would you mind changing GetSymbolLocation -> getSymbolLocation?
Comment at: clangd/index/SymbolYAML.cpp:51
IO.mapRequired("EndOffset", Value.EndOffset);
-IO.mapRequired("FilePath", Value.FilePath);
+IO.mapRequired("FileUri", Value.FileUri);
}
more URI
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42915
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42915: [clangd] Use URIs in index symbols.
ioeric created this revision.
ioeric added reviewers: hokein, sammccall.
Herald added subscribers: cfe-commits, jkorous-apple, ilya-biryukov, klimek.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42915
Files:
clangd/index/Index.cpp
clangd/index/Index.h
clangd/index/Merge.cpp
clangd/index/SymbolCollector.cpp
clangd/index/SymbolYAML.cpp
unittests/clangd/IndexTests.cpp
unittests/clangd/SymbolCollectorTests.cpp
Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -46,7 +46,7 @@
return arg.CompletionSnippetInsertText == S;
}
MATCHER_P(QName, Name, "") { return (arg.Scope + arg.Name).str() == Name; }
-MATCHER_P(CPath, P, "") { return arg.CanonicalDeclaration.FilePath == P; }
+MATCHER_P(Uri, P, "") { return arg.CanonicalDeclaration.FileUri == P; }
MATCHER_P(LocationOffsets, Offsets, "") {
// Offset range in SymbolLocation is [start, end] while in Clangd is [start,
// end).
@@ -58,8 +58,6 @@
namespace clangd {
namespace {
-const char TestHeaderName[] = "symbols.h";
-const char TestFileName[] = "symbol.cc";
class SymbolIndexActionFactory : public tooling::FrontendActionFactory {
public:
SymbolIndexActionFactory(SymbolCollector::Options COpts)
@@ -82,6 +80,13 @@
class SymbolCollectorTest : public ::testing::Test {
public:
+ SymbolCollectorTest()
+ : TestHeaderName(getVirtualTestFilePath("symbol.h").str()),
+TestFileName(getVirtualTestFilePath("symbol.cc").str()) {
+TestHeaderUri = URI::createFile(TestHeaderName).toString();
+TestFileUri = URI::createFile(TestFileName).toString();
+ }
+
bool runSymbolCollector(StringRef HeaderCode, StringRef MainCode) {
llvm::IntrusiveRefCntPtr InMemoryFileSystem(
new vfs::InMemoryFileSystem);
@@ -100,15 +105,21 @@
std::string Content = MainCode;
if (!HeaderCode.empty())
- Content = "#include\"" + std::string(TestHeaderName) + "\"\n" + Content;
+ Content = (llvm::Twine("#include\"") +
+ llvm::sys::path::filename(TestHeaderName) + "\"\n" + Content)
+.str();
InMemoryFileSystem->addFile(TestFileName, 0,
llvm::MemoryBuffer::getMemBuffer(Content));
Invocation.run();
Symbols = Factory->Collector->takeSymbols();
return true;
}
protected:
+ std::string TestHeaderName;
+ std::string TestHeaderUri;
+ std::string TestFileName;
+ std::string TestFileUri;
SymbolSlab Symbols;
SymbolCollector::Options CollectorOpts;
};
@@ -165,16 +176,19 @@
CollectorOpts.IndexMainFiles = false;
runSymbolCollector("class Foo {};", /*Main=*/"");
EXPECT_THAT(Symbols,
- UnorderedElementsAre(AllOf(QName("Foo"), CPath("symbols.h";
+ UnorderedElementsAre(AllOf(QName("Foo"), Uri(TestHeaderUri;
}
TEST_F(SymbolCollectorTest, SymbolRelativeWithFallback) {
CollectorOpts.IndexMainFiles = false;
+ TestHeaderName = "x.h";
+ TestFileName = "x.cpp";
+ TestHeaderUri =
+ URI::createFile(getVirtualTestFilePath(TestHeaderName)).toString();
CollectorOpts.FallbackDir = getVirtualTestRoot();
runSymbolCollector("class Foo {};", /*Main=*/"");
EXPECT_THAT(Symbols,
- UnorderedElementsAre(AllOf(
- QName("Foo"), CPath(getVirtualTestFilePath("symbols.h");
+ UnorderedElementsAre(AllOf(QName("Foo"), Uri(TestHeaderUri;
}
TEST_F(SymbolCollectorTest, IncludeEnums) {
@@ -229,14 +243,14 @@
)");
runSymbolCollector(Header.code(), /*Main=*/"");
- EXPECT_THAT(Symbols,
- UnorderedElementsAre(
- AllOf(QName("abc_Test"),
-LocationOffsets(Header.offsetRange("expansion")),
-CPath(TestHeaderName)),
- AllOf(QName("Test"),
-LocationOffsets(Header.offsetRange("spelling")),
-CPath(TestHeaderName;
+ EXPECT_THAT(
+ Symbols,
+ UnorderedElementsAre(
+ AllOf(QName("abc_Test"),
+LocationOffsets(Header.offsetRange("expansion")),
+Uri(TestHeaderUri)),
+ AllOf(QName("Test"), LocationOffsets(Header.offsetRange("spelling")),
+Uri(TestHeaderUri;
}
TEST_F(SymbolCollectorTest, SymbolFormedFromMacroInMainFile) {
@@ -254,14 +268,13 @@
FF2();
)");
runSymbolCollector(/*Header=*/"", Main.code());
- EXPECT_THAT(Symbols,
- UnorderedElementsAre(
- AllOf(QName("abc_Test"),
-LocationOffsets(Main.offsetRange("expansion")),
-CPath(TestFileName)),
- AllOf(QName("Test"),
-LocationOffsets(Main.offsetRange("spelling")),
-CPath(TestFileName;
+ EXPECT_THAT(Symbols, UnorderedElementsAre(
+
