[PATCH] D38639: [clangd] #include statements support for Open definition
This revision was automatically updated to reflect the committed changes.
Closed by commit rL325662: [clangd] #include statements support for Open
definition (authored by malaperle, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D38639?vs=134549&id=135187#toc
Repository:
rL LLVM
https://reviews.llvm.org/D38639
Files:
clang-tools-extra/trunk/clangd/ClangdUnit.cpp
clang-tools-extra/trunk/clangd/ClangdUnit.h
clang-tools-extra/trunk/clangd/Protocol.h
clang-tools-extra/trunk/clangd/SourceCode.cpp
clang-tools-extra/trunk/clangd/SourceCode.h
clang-tools-extra/trunk/clangd/XRefs.cpp
clang-tools-extra/trunk/unittests/clangd/XRefsTests.cpp
Index: clang-tools-extra/trunk/unittests/clangd/XRefsTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/XRefsTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/XRefsTests.cpp
@@ -36,6 +36,7 @@
namespace {
using testing::ElementsAre;
using testing::Field;
+using testing::IsEmpty;
using testing::Matcher;
using testing::UnorderedElementsAreArray;
@@ -563,6 +564,73 @@
}
}
+TEST(GoToInclude, All) {
+ MockFSProvider FS;
+ IgnoreDiagnostics DiagConsumer;
+ MockCompilationDatabase CDB;
+
+ ClangdServer Server(CDB, DiagConsumer, FS, getDefaultAsyncThreadsCount(),
+ /*StorePreamblesInMemory=*/true);
+
+ auto FooCpp = testPath("foo.cpp");
+ const char *SourceContents = R"cpp(
+ #include ^"$2^foo.h$3^"
+ #include "$4^invalid.h"
+ int b = a;
+ // test
+ int foo;
+ #in$5^clude "$6^foo.h"$7^
+ )cpp";
+ Annotations SourceAnnotations(SourceContents);
+ FS.Files[FooCpp] = SourceAnnotations.code();
+ auto FooH = testPath("foo.h");
+ auto FooHUri = URIForFile{FooH};
+
+ const char *HeaderContents = R"cpp([[]]int a;)cpp";
+ Annotations HeaderAnnotations(HeaderContents);
+ FS.Files[FooH] = HeaderAnnotations.code();
+
+ Server.addDocument(FooH, HeaderAnnotations.code());
+ Server.addDocument(FooCpp, SourceAnnotations.code());
+
+ // Test include in preamble.
+ auto Locations =
+ runFindDefinitions(Server, FooCpp, SourceAnnotations.point());
+ ASSERT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+ EXPECT_THAT(Locations->Value,
+ ElementsAre(Location{FooHUri, HeaderAnnotations.range()}));
+
+ // Test include in preamble, last char.
+ Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("2"));
+ ASSERT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+ EXPECT_THAT(Locations->Value,
+ ElementsAre(Location{FooHUri, HeaderAnnotations.range()}));
+
+ Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("3"));
+ ASSERT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+ EXPECT_THAT(Locations->Value,
+ ElementsAre(Location{FooHUri, HeaderAnnotations.range()}));
+
+ // Test include outside of preamble.
+ Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("6"));
+ ASSERT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+ EXPECT_THAT(Locations->Value,
+ ElementsAre(Location{FooHUri, HeaderAnnotations.range()}));
+
+ // Test a few positions that do not result in Locations.
+ Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("4"));
+ ASSERT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+ EXPECT_THAT(Locations->Value, IsEmpty());
+
+ Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("5"));
+ ASSERT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+ EXPECT_THAT(Locations->Value, IsEmpty());
+
+ Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("7"));
+ ASSERT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+ EXPECT_THAT(Locations->Value, IsEmpty());
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/trunk/clangd/ClangdUnit.h
===
--- clang-tools-extra/trunk/clangd/ClangdUnit.h
+++ clang-tools-extra/trunk/clangd/ClangdUnit.h
@@ -45,15 +45,19 @@
llvm::SmallVector FixIts;
};
+using InclusionLocations = std::vector>;
+
// Stores Preamble and associated data.
struct PreambleData {
PreambleData(PrecompiledPreamble Preamble,
std::vector TopLevelDeclIDs,
- std::vector Diags);
+ std::vector Diags,
+ InclusionLocations IncLocations);
PrecompiledPreamble Preamble;
std::vector TopLevelDeclIDs;
std::vector Diags;
+ InclusionLocations IncLocations;
};
/// Information required to run clang, e.g. to parse AST or do code completion.
@@ -97,13 +101,14 @@
/// Returns the esitmated size of the AST and the accessory structures, in
/// bytes. Does not include the size of the preamble.
std::size_t getUsedBytes() const;
+ const InclusionLocatio
[PATCH] D38639: [clangd] #include statements support for Open definition
malaperle updated this revision to Diff 134549.
malaperle added a comment.
Change some EXPECT_TRUE to ASSERT_TRUE
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D38639
Files:
clangd/ClangdUnit.cpp
clangd/ClangdUnit.h
clangd/Protocol.h
clangd/SourceCode.cpp
clangd/SourceCode.h
clangd/XRefs.cpp
unittests/clangd/XRefsTests.cpp
Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -36,6 +36,7 @@
namespace {
using testing::ElementsAre;
using testing::Field;
+using testing::IsEmpty;
using testing::Matcher;
using testing::UnorderedElementsAreArray;
@@ -259,6 +260,73 @@
SourceAnnotations.range()}));
}
+TEST(GoToInclude, All) {
+ MockFSProvider FS;
+ IgnoreDiagnostics DiagConsumer;
+ MockCompilationDatabase CDB;
+
+ ClangdServer Server(CDB, DiagConsumer, FS, getDefaultAsyncThreadsCount(),
+ /*StorePreamblesInMemory=*/true);
+
+ auto FooCpp = getVirtualTestFilePath("foo.cpp");
+ const char *SourceContents = R"cpp(
+ #include ^"$2^foo.h$3^"
+ #include "$4^invalid.h"
+ int b = a;
+ // test
+ int foo;
+ #in$5^clude "$6^foo.h"$7^
+ )cpp";
+ Annotations SourceAnnotations(SourceContents);
+ FS.Files[FooCpp] = SourceAnnotations.code();
+ auto FooH = getVirtualTestFilePath("foo.h");
+ auto FooHUri = URIForFile{FooH.str()};
+
+ const char *HeaderContents = R"cpp([[]]int a;)cpp";
+ Annotations HeaderAnnotations(HeaderContents);
+ FS.Files[FooH] = HeaderAnnotations.code();
+
+ Server.addDocument(FooH, HeaderAnnotations.code());
+ Server.addDocument(FooCpp, SourceAnnotations.code());
+
+ // Test include in preamble.
+ auto Locations =
+ runFindDefinitions(Server, FooCpp, SourceAnnotations.point());
+ ASSERT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+ EXPECT_THAT(Locations->Value,
+ ElementsAre(Location{FooHUri, HeaderAnnotations.range()}));
+
+ // Test include in preamble, last char.
+ Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("2"));
+ ASSERT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+ EXPECT_THAT(Locations->Value,
+ ElementsAre(Location{FooHUri, HeaderAnnotations.range()}));
+
+ Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("3"));
+ ASSERT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+ EXPECT_THAT(Locations->Value,
+ ElementsAre(Location{FooHUri, HeaderAnnotations.range()}));
+
+ // Test include outside of preamble.
+ Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("6"));
+ ASSERT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+ EXPECT_THAT(Locations->Value,
+ ElementsAre(Location{FooHUri, HeaderAnnotations.range()}));
+
+ // Test a few positions that do not result in Locations.
+ Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("4"));
+ ASSERT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+ EXPECT_THAT(Locations->Value, IsEmpty());
+
+ Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("5"));
+ ASSERT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+ EXPECT_THAT(Locations->Value, IsEmpty());
+
+ Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("7"));
+ ASSERT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+ EXPECT_THAT(Locations->Value, IsEmpty());
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clangd/XRefs.cpp
===
--- clangd/XRefs.cpp
+++ clangd/XRefs.cpp
@@ -8,6 +8,7 @@
//===-===//
#include "XRefs.h"
#include "Logger.h"
+#include "SourceCode.h"
#include "URI.h"
#include "clang/Index/IndexDataConsumer.h"
#include "clang/Index/IndexingAction.h"
@@ -130,12 +131,8 @@
return llvm::None;
SourceLocation LocEnd = Lexer::getLocForEndOfToken(ValSourceRange.getEnd(), 0,
SourceMgr, LangOpts);
- Position Begin;
- Begin.line = SourceMgr.getSpellingLineNumber(LocStart) - 1;
- Begin.character = SourceMgr.getSpellingColumnNumber(LocStart) - 1;
- Position End;
- End.line = SourceMgr.getSpellingLineNumber(LocEnd) - 1;
- End.character = SourceMgr.getSpellingColumnNumber(LocEnd) - 1;
+ Position Begin = sourceLocToPosition(SourceMgr, LocStart);
+ Position End = sourceLocToPosition(SourceMgr, LocEnd);
Range R = {Begin, End};
Location L;
@@ -193,6 +190,15 @@
Result.push_back(*L);
}
+ /// Process targets for paths inside #include directive.
+ for (auto &IncludeLoc : AST.getInclusionLocations()) {
+Range R = IncludeLoc.first;
+Position Pos = sourceLocToPosition(SourceMgr, SourceLocationBeg);
+
+if (R.conta
[PATCH] D38639: [clangd] #include statements support for Open definition
ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM modulo the `ASSERT_TRUE` nit. Comment at: unittests/clangd/XRefsTests.cpp:295 + runFindDefinitions(Server, FooCpp, SourceAnnotations.point()); + EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error"; + EXPECT_THAT(Locations->Value, Maybe replace with `ASSERT_TRUE`? If `Locations` has an error, an assertion will fire in debug mode inside `Expected` that the error was not handled. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D38639 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38639: [clangd] #include statements support for Open definition
malaperle updated this revision to Diff 134432.
malaperle added a comment.
Use EXPECT_THAT in a few places and clean-ups in tests.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D38639
Files:
clangd/ClangdUnit.cpp
clangd/ClangdUnit.h
clangd/Protocol.h
clangd/SourceCode.cpp
clangd/SourceCode.h
clangd/XRefs.cpp
unittests/clangd/XRefsTests.cpp
Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -36,6 +36,7 @@
namespace {
using testing::ElementsAre;
using testing::Field;
+using testing::IsEmpty;
using testing::Matcher;
using testing::UnorderedElementsAreArray;
@@ -259,6 +260,73 @@
SourceAnnotations.range()}));
}
+TEST(GoToInclude, All) {
+ MockFSProvider FS;
+ IgnoreDiagnostics DiagConsumer;
+ MockCompilationDatabase CDB;
+
+ ClangdServer Server(CDB, DiagConsumer, FS, getDefaultAsyncThreadsCount(),
+ /*StorePreamblesInMemory=*/true);
+
+ auto FooCpp = getVirtualTestFilePath("foo.cpp");
+ const char *SourceContents = R"cpp(
+ #include ^"$2^foo.h$3^"
+ #include "$4^invalid.h"
+ int b = a;
+ // test
+ int foo;
+ #in$5^clude "$6^foo.h"$7^
+ )cpp";
+ Annotations SourceAnnotations(SourceContents);
+ FS.Files[FooCpp] = SourceAnnotations.code();
+ auto FooH = getVirtualTestFilePath("foo.h");
+ auto FooHUri = URIForFile{FooH.str()};
+
+ const char *HeaderContents = R"cpp([[]]int a;)cpp";
+ Annotations HeaderAnnotations(HeaderContents);
+ FS.Files[FooH] = HeaderAnnotations.code();
+
+ Server.addDocument(FooH, HeaderAnnotations.code());
+ Server.addDocument(FooCpp, SourceAnnotations.code());
+
+ // Test include in preamble.
+ auto Locations =
+ runFindDefinitions(Server, FooCpp, SourceAnnotations.point());
+ EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+ EXPECT_THAT(Locations->Value,
+ ElementsAre(Location{FooHUri, HeaderAnnotations.range()}));
+
+ // Test include in preamble, last char.
+ Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("2"));
+ EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+ EXPECT_THAT(Locations->Value,
+ ElementsAre(Location{FooHUri, HeaderAnnotations.range()}));
+
+ Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("3"));
+ EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+ EXPECT_THAT(Locations->Value,
+ ElementsAre(Location{FooHUri, HeaderAnnotations.range()}));
+
+ // Test include outside of preamble.
+ Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("6"));
+ EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+ EXPECT_THAT(Locations->Value,
+ ElementsAre(Location{FooHUri, HeaderAnnotations.range()}));
+
+ // Test a few positions that do not result in Locations.
+ Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("4"));
+ EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+ EXPECT_THAT(Locations->Value, IsEmpty());
+
+ Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("5"));
+ EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+ EXPECT_THAT(Locations->Value, IsEmpty());
+
+ Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("7"));
+ EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+ EXPECT_THAT(Locations->Value, IsEmpty());
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clangd/XRefs.cpp
===
--- clangd/XRefs.cpp
+++ clangd/XRefs.cpp
@@ -8,6 +8,7 @@
//===-===//
#include "XRefs.h"
#include "Logger.h"
+#include "SourceCode.h"
#include "URI.h"
#include "clang/Index/IndexDataConsumer.h"
#include "clang/Index/IndexingAction.h"
@@ -130,12 +131,8 @@
return llvm::None;
SourceLocation LocEnd = Lexer::getLocForEndOfToken(ValSourceRange.getEnd(), 0,
SourceMgr, LangOpts);
- Position Begin;
- Begin.line = SourceMgr.getSpellingLineNumber(LocStart) - 1;
- Begin.character = SourceMgr.getSpellingColumnNumber(LocStart) - 1;
- Position End;
- End.line = SourceMgr.getSpellingLineNumber(LocEnd) - 1;
- End.character = SourceMgr.getSpellingColumnNumber(LocEnd) - 1;
+ Position Begin = sourceLocToPosition(SourceMgr, LocStart);
+ Position End = sourceLocToPosition(SourceMgr, LocEnd);
Range R = {Begin, End};
Location L;
@@ -193,6 +190,15 @@
Result.push_back(*L);
}
+ /// Process targets for paths inside #include directive.
+ for (auto &IncludeLoc : AST.getInclusionLocations()) {
+Range R = IncludeLoc.first;
+Position Pos = sourceLocToPosition(SourceMgr, SourceLocationBeg);
+
[PATCH] D38639: [clangd] #include statements support for Open definition
ilya-biryukov added a comment.
Last drop of NITs. After they're fixed, this change is ready to land!
Comment at: unittests/clangd/XRefsTests.cpp:282
+ const char *HeaderContents = R"cpp([[]]int a;)cpp";
+ Annotations HeaderAnnoations(HeaderContents);
+ FS.Files[FooH] = HeaderAnnoations.code();
There's a typo, should be `HeaderAnnotations`
Comment at: unittests/clangd/XRefsTests.cpp:302
+ Locations = ExpectedLocations->Value;
+ EXPECT_TRUE(!Locations.empty());
+ ASSERT_EQ(Locations[0].uri.file, FooH);
NIT: one assertion using `EXPECT_THAT` will read better and produce more
helpful messages in case of failures:
```
// Create FooHUri to avoid typing URIForFile everywhere.
auto FooHUri = URIForFile{FooH.str()};
EXPECT_THAT(Locations, ElementsAre(Location{FooHUri,
HeaderAnnoations.range()}));
```
Comment at: unittests/clangd/XRefsTests.cpp:328
+ Locations = ExpectedLocations->Value;
+ EXPECT_TRUE(Locations.empty());
+
NIT: Use `EXPECT_THAT` here too:
`EXPECT_THAT(Locations, IsEmpty());`
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D38639
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38639: [clangd] #include statements support for Open definition
malaperle updated this revision to Diff 134296.
malaperle added a comment.
Fix some NITs, add more tests.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D38639
Files:
clangd/ClangdUnit.cpp
clangd/ClangdUnit.h
clangd/Protocol.h
clangd/SourceCode.cpp
clangd/SourceCode.h
clangd/XRefs.cpp
unittests/clangd/XRefsTests.cpp
Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -257,6 +257,89 @@
SourceAnnotations.range()}));
}
+TEST(GoToInclude, All) {
+ MockFSProvider FS;
+ IgnoreDiagnostics DiagConsumer;
+ MockCompilationDatabase CDB;
+
+ ClangdServer Server(CDB, DiagConsumer, FS, getDefaultAsyncThreadsCount(),
+ /*StorePreamblesInMemory=*/true);
+
+ auto FooCpp = getVirtualTestFilePath("foo.cpp");
+ const char *SourceContents = R"cpp(
+ #include ^"$2^foo.h$3^"
+ #include "$4^invalid.h"
+ int b = a;
+ // test
+ int foo;
+ #in$5^clude "$6^foo.h"$7^
+ )cpp";
+ Annotations SourceAnnoations(SourceContents);
+ FS.Files[FooCpp] = SourceAnnoations.code();
+ auto FooH = getVirtualTestFilePath("foo.h");
+
+ const char *HeaderContents = R"cpp([[]]int a;)cpp";
+ Annotations HeaderAnnoations(HeaderContents);
+ FS.Files[FooH] = HeaderAnnoations.code();
+
+ Server.addDocument(FooH, HeaderAnnoations.code());
+ Server.addDocument(FooCpp, SourceAnnoations.code());
+
+ // Test include in preamble.
+ auto ExpectedLocations =
+ Server.findDefinitions(FooCpp, SourceAnnoations.point());
+ ASSERT_TRUE(!!ExpectedLocations);
+ std::vector Locations = ExpectedLocations->Value;
+ EXPECT_TRUE(!Locations.empty());
+ ASSERT_EQ(Locations[0].uri.file, FooH);
+ ASSERT_EQ(Locations[0].range, HeaderAnnoations.range());
+
+ // Test include in preamble, last char.
+ ExpectedLocations =
+ Server.findDefinitions(FooCpp, SourceAnnoations.point("2"));
+ ASSERT_TRUE(!!ExpectedLocations);
+ Locations = ExpectedLocations->Value;
+ EXPECT_TRUE(!Locations.empty());
+ ASSERT_EQ(Locations[0].uri.file, FooH);
+ ASSERT_EQ(Locations[0].range, HeaderAnnoations.range());
+
+ ExpectedLocations =
+ Server.findDefinitions(FooCpp, SourceAnnoations.point("3"));
+ ASSERT_TRUE(!!ExpectedLocations);
+ Locations = ExpectedLocations->Value;
+ EXPECT_TRUE(!Locations.empty());
+ ASSERT_EQ(Locations[0].uri.file, FooH);
+ ASSERT_EQ(Locations[0].range, HeaderAnnoations.range());
+
+ // Test include outside of preamble.
+ ExpectedLocations =
+ Server.findDefinitions(FooCpp, SourceAnnoations.point("6"));
+ ASSERT_TRUE(!!ExpectedLocations);
+ Locations = ExpectedLocations->Value;
+ EXPECT_TRUE(!Locations.empty());
+ ASSERT_EQ(Locations[0].uri.file, FooH);
+ ASSERT_EQ(Locations[0].range, HeaderAnnoations.range());
+
+ // Test a few positions that do not result in Locations.
+ ExpectedLocations =
+ Server.findDefinitions(FooCpp, SourceAnnoations.point("4"));
+ ASSERT_TRUE(!!ExpectedLocations);
+ Locations = ExpectedLocations->Value;
+ EXPECT_TRUE(Locations.empty());
+
+ ExpectedLocations =
+ Server.findDefinitions(FooCpp, SourceAnnoations.point("5"));
+ ASSERT_TRUE(!!ExpectedLocations);
+ Locations = ExpectedLocations->Value;
+ EXPECT_TRUE(Locations.empty());
+
+ ExpectedLocations =
+ Server.findDefinitions(FooCpp, SourceAnnoations.point("7"));
+ ASSERT_TRUE(!!ExpectedLocations);
+ Locations = ExpectedLocations->Value;
+ EXPECT_TRUE(Locations.empty());
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clangd/XRefs.cpp
===
--- clangd/XRefs.cpp
+++ clangd/XRefs.cpp
@@ -8,6 +8,7 @@
//===-===//
#include "XRefs.h"
#include "Logger.h"
+#include "SourceCode.h"
#include "URI.h"
#include "clang/Index/IndexDataConsumer.h"
#include "clang/Index/IndexingAction.h"
@@ -130,12 +131,8 @@
return llvm::None;
SourceLocation LocEnd = Lexer::getLocForEndOfToken(ValSourceRange.getEnd(), 0,
SourceMgr, LangOpts);
- Position Begin;
- Begin.line = SourceMgr.getSpellingLineNumber(LocStart) - 1;
- Begin.character = SourceMgr.getSpellingColumnNumber(LocStart) - 1;
- Position End;
- End.line = SourceMgr.getSpellingLineNumber(LocEnd) - 1;
- End.character = SourceMgr.getSpellingColumnNumber(LocEnd) - 1;
+ Position Begin = sourceLocToPosition(SourceMgr, LocStart);
+ Position End = sourceLocToPosition(SourceMgr, LocEnd);
Range R = {Begin, End};
Location L;
@@ -193,6 +190,15 @@
Result.push_back(*L);
}
+ /// Process targets for paths inside #include directive.
+ for (auto &IncludeLoc : AST.getInclusionLocations()) {
+Range R = IncludeLoc.first;
+Position Pos = sourceLocToPosition(SourceMgr, SourceLocationBeg);
+
+if (R.contains(Pos))
+ Result
[PATCH] D38639: [clangd] #include statements support for Open definition
ilya-biryukov added inline comments.
Comment at: unittests/clangd/XRefsTests.cpp:53
+class IgnoreDiagnostics : public DiagnosticsConsumer {
+ void onDiagnosticsReady(
malaperle wrote:
> ilya-biryukov wrote:
> > NIT: remove this class, use `IgnoreDiagnostics` from `Compiler.h` instead.
> It looks like IgnoreDiagnostics from Compiler.h implements DiagnosticConsumer
> from Compiler.h, and not DiagnosticsConsumer from ClangdServer.h so that
> wouldn't work.
You're right, I got confused, sorry. Disregard my comment.
Comment at: unittests/clangd/XRefsTests.cpp:245
+ const char *SourceContents = R"cpp(
+ #include "$2^invalid.h"
+ #include "^foo.h"
malaperle wrote:
> ilya-biryukov wrote:
> > Could we also add tests for corner cases: cursor before opening quote,
> > cursor after the closing quote, cursor in the middle of `#include` token?
> > (we shouldn't navigate anywhere in the middle of the #include token)
> > cursor before opening quote, cursor after the closing quote
>
> I assume we don't want to navigate anywhere for these positions? I don't have
> an opinion.
>
> > (we shouldn't navigate anywhere in the middle of the #include token)
>
> It did in CDT and I thought it worked nicely as it made it easier to click on
> it. You can hold ctrl on the whole line and it underlined it (well except
> trailing comments). But clients don't handle the spaces nicely (underline
> between #include and file name) so I thought I'd work on the client first
> before making the server do it. Anyhow, for now it shouldn't navigate indeed.
> > cursor before opening quote, cursor after the closing quote
>I assume we don't want to navigate anywhere for these positions? I don't have
>an opinion.
I'd say we should navigate when the cursor is right before the opening quote.
For the closing quote I don't have any opinions either :-)
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D38639
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38639: [clangd] #include statements support for Open definition
malaperle added inline comments. Comment at: unittests/clangd/XRefsTests.cpp:245 + const char *SourceContents = R"cpp( + #include "$2^invalid.h" + #include "^foo.h" ilya-biryukov wrote: > Could we also add tests for corner cases: cursor before opening quote, cursor > after the closing quote, cursor in the middle of `#include` token? (we > shouldn't navigate anywhere in the middle of the #include token) > cursor before opening quote, cursor after the closing quote I assume we don't want to navigate anywhere for these positions? I don't have an opinion. > (we shouldn't navigate anywhere in the middle of the #include token) It did in CDT and I thought it worked nicely as it made it easier to click on it. You can hold ctrl on the whole line and it underlined it (well except trailing comments). But clients don't handle the spaces nicely (underline between #include and file name) so I thought I'd work on the client first before making the server do it. Anyhow, for now it shouldn't navigate indeed. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D38639 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38639: [clangd] #include statements support for Open definition
malaperle added inline comments.
Comment at: unittests/clangd/XRefsTests.cpp:53
+class IgnoreDiagnostics : public DiagnosticsConsumer {
+ void onDiagnosticsReady(
ilya-biryukov wrote:
> NIT: remove this class, use `IgnoreDiagnostics` from `Compiler.h` instead.
It looks like IgnoreDiagnostics from Compiler.h implements DiagnosticConsumer
from Compiler.h, and not DiagnosticsConsumer from ClangdServer.h so that
wouldn't work.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D38639
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38639: [clangd] #include statements support for Open definition
ilya-biryukov added inline comments.
Comment at: clangd/ClangdUnit.cpp:103
+
+if (SourceMgr.isInMainFile(FilenameRange.getAsRange().getBegin())) {
+ // Only inclusion directives in the main file make sense. The user cannot
NIT: replace `FilenameRange.getAsRange()` with `SR`
Comment at: clangd/ClangdUnit.cpp:126
+
+ CppFilePreambleCallbacks() : SourceMgr(nullptr) {}
+
NIT: remove ctor, use initializer on the member instead:
```
private:
SourceManager *SourceMgr = nullptr;
```
Comment at: clangd/ClangdUnit.cpp:160
+ SourceManager *SourceMgr;
+ InclusionLocations IncLocations;
};
Maybe swap `IncLocations` and `SourceMgr`? Grouping `TopLevelDecls`,
`TopLevelDeclIDs` and `IncLocations` together seems reasonable, as all of them
are essentially output parameters.
Comment at: clangd/ClangdUnit.cpp:296
StoreDiagsConsumer UnitDiagsConsumer(/*ref*/ ASTDiags);
+ InclusionLocations IncLocations;
+ // Copy over the includes from the preamble, then combine with the
NIT: move `IncLocations` closer to their first use, i.e. create the variable
right before `addPPCallbacks()`
Comment at: clangd/ClangdUnit.h:51
+using IncludeReferenceMap = std::unordered_map;
+
malaperle wrote:
> ilya-biryukov wrote:
> > We use `unordered_map` as a `vector>` here. (i.e. we never look up
> > values by key, because we don't only the range, merely a point in the range)
> > Replace map with `vector>` and remove `RangeHash` that we don't need
> > anymore.
> Done. I also renamed, IncludeReferenceMap to InclusionLocations. I thought I
> was more suitable.
LG
Comment at: clangd/ClangdUnit.h:105
+ const InclusionLocations &getInclusionLocations() const {
+return IncLocations;
+ };
NIT: move to .cpp file to be consisten with other decls in this file.
Comment at: clangd/XRefs.cpp:171
+Range R = IncludeLoc.first;
+const SourceManager &SourceMgr = AST.getASTContext().getSourceManager();
+Position Pos = sourceLocToPosition(SourceMgr, SourceLocationBeg);
NIT: `SourceMgr` does not depend on the loop variable, declare it out of the
loop.
Comment at: clangd/XRefs.cpp:174
+
+if (R.start.line == Pos.line && R.start.character <= Pos.character &&
+Pos.character <= R.end.character) {
NIT: define `operator <=` for `Position` and do this instead: `R.start <= Pos
&& Pos < R.end` (note that LSP ranges are half-open, i.e. the end is excluded).
Even better: define a `bool contains(Position Pos) const` helper in the `Range`
class and use it here (will abstract away to half-open nature of the range)
Comment at: clangd/XRefs.cpp:178
+Range{Position{0, 0}, Position{0,
0}}});
+ return IncludeTargets;
+}
Let's follow the pattern of decls and macros here. I.e. not return from the
function, merely collect all the results.
Comment at: unittests/clangd/XRefsTests.cpp:53
+class IgnoreDiagnostics : public DiagnosticsConsumer {
+ void onDiagnosticsReady(
NIT: remove this class, use `IgnoreDiagnostics` from `Compiler.h` instead.
Comment at: unittests/clangd/XRefsTests.cpp:245
+ const char *SourceContents = R"cpp(
+ #include "$2^invalid.h"
+ #include "^foo.h"
Could we also add tests for corner cases: cursor before opening quote, cursor
after the closing quote, cursor in the middle of `#include` token? (we
shouldn't navigate anywhere in the middle of the #include token)
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D38639
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38639: [clangd] #include statements support for Open definition
malaperle added inline comments.
Comment at: clangd/ClangdUnit.h:51
+using IncludeReferenceMap = std::unordered_map;
+
ilya-biryukov wrote:
> We use `unordered_map` as a `vector>` here. (i.e. we never look up
> values by key, because we don't only the range, merely a point in the range)
> Replace map with `vector>` and remove `RangeHash` that we don't need
> anymore.
Done. I also renamed, IncludeReferenceMap to InclusionLocations. I thought I
was more suitable.
Comment at: unittests/clangd/ClangdTests.cpp:781
+
+ Position P = Position{1, 11};
+
ilya-biryukov wrote:
> We moved `findDefinition` tests to `XRefsTests.cpp` and added a useful
> `Annotation` helper to make writing these tests simpler.
> Maybe we could use it for this test as well?
Done. (I don't know why I can't use the "done" checkbox, maybe because I wasn't
the original author?)
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D38639
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38639: [clangd] #include statements support for Open definition
malaperle updated this revision to Diff 133978.
malaperle added a comment.
Herald added subscribers: ioeric, jkorous-apple.
Move tests to XRefsTests, change IncludeReferenceMap to a vector and rename it.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D38639
Files:
clangd/ClangdUnit.cpp
clangd/ClangdUnit.h
clangd/SourceCode.cpp
clangd/SourceCode.h
clangd/XRefs.cpp
unittests/clangd/XRefsTests.cpp
Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -50,6 +50,11 @@
return std::move(*AST);
}
+class IgnoreDiagnostics : public DiagnosticsConsumer {
+ void onDiagnosticsReady(
+ PathRef File, Tagged> Diagnostics) override {}
+};
+
// Extracts ranges from an annotated example, and constructs a matcher for a
// highlight set. Ranges should be named $read/$write as appropriate.
Matcher &>
@@ -227,6 +232,59 @@
}
}
+TEST(GoToInclude, All) {
+ MockFSProvider FS;
+ IgnoreDiagnostics DiagConsumer;
+ MockCompilationDatabase CDB;
+
+ ClangdServer Server(CDB, DiagConsumer, FS, getDefaultAsyncThreadsCount(),
+ /*StorePreamblesInMemory=*/true);
+
+ auto FooCpp = getVirtualTestFilePath("foo.cpp");
+ const char *SourceContents = R"cpp(
+ #include "$2^invalid.h"
+ #include "^foo.h"
+ int b = a;
+ // test
+ int foo;
+ #include "$3^foo.h"
+ )cpp";
+ Annotations SourceAnnoations(SourceContents);
+ FS.Files[FooCpp] = SourceAnnoations.code();
+ auto FooH = getVirtualTestFilePath("foo.h");
+
+ const char *HeaderContents = R"cpp([[]]int a;)cpp";
+ Annotations HeaderAnnoations(HeaderContents);
+ FS.Files[FooH] = HeaderAnnoations.code();
+
+ Server.addDocument(FooH, HeaderAnnoations.code());
+ Server.addDocument(FooCpp, SourceAnnoations.code());
+
+ // Test include in preamble
+ auto ExpectedLocations =
+ Server.findDefinitions(FooCpp, SourceAnnoations.point());
+ ASSERT_TRUE(!!ExpectedLocations);
+ std::vector Locations = ExpectedLocations->Value;
+ EXPECT_TRUE(!Locations.empty());
+ ASSERT_EQ(Locations[0].uri.file, FooH);
+ ASSERT_EQ(Locations[0].range, HeaderAnnoations.range());
+
+ ExpectedLocations =
+ Server.findDefinitions(FooCpp, SourceAnnoations.point("2"));
+ ASSERT_TRUE(!!ExpectedLocations);
+ Locations = ExpectedLocations->Value;
+ EXPECT_TRUE(Locations.empty());
+
+ // Test include outside of Preamble
+ ExpectedLocations =
+ Server.findDefinitions(FooCpp, SourceAnnoations.point("3"));
+ ASSERT_TRUE(!!ExpectedLocations);
+ Locations = ExpectedLocations->Value;
+ EXPECT_TRUE(!Locations.empty());
+ ASSERT_EQ(Locations[0].uri.file, FooH);
+ ASSERT_EQ(Locations[0].range, HeaderAnnoations.range());
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clangd/XRefs.cpp
===
--- clangd/XRefs.cpp
+++ clangd/XRefs.cpp
@@ -8,6 +8,7 @@
//===-===//
#include "XRefs.h"
#include "Logger.h"
+#include "SourceCode.h"
#include "URI.h"
#include "clang/Index/IndexDataConsumer.h"
#include "clang/Index/IndexingAction.h"
@@ -129,12 +130,8 @@
return llvm::None;
SourceLocation LocEnd = Lexer::getLocForEndOfToken(ValSourceRange.getEnd(), 0,
SourceMgr, LangOpts);
- Position Begin;
- Begin.line = SourceMgr.getSpellingLineNumber(LocStart) - 1;
- Begin.character = SourceMgr.getSpellingColumnNumber(LocStart) - 1;
- Position End;
- End.line = SourceMgr.getSpellingLineNumber(LocEnd) - 1;
- End.character = SourceMgr.getSpellingColumnNumber(LocEnd) - 1;
+ Position Begin = sourceLocToPosition(SourceMgr, LocStart);
+ Position End = sourceLocToPosition(SourceMgr, LocEnd);
Range R = {Begin, End};
Location L;
@@ -167,6 +164,21 @@
indexTopLevelDecls(AST.getASTContext(), AST.getTopLevelDecls(),
DeclMacrosFinder, IndexOpts);
+ /// Process targets for paths inside #include directive.
+ std::vector IncludeTargets;
+ for (auto &IncludeLoc : AST.getInclusionLocations()) {
+Range R = IncludeLoc.first;
+const SourceManager &SourceMgr = AST.getASTContext().getSourceManager();
+Position Pos = sourceLocToPosition(SourceMgr, SourceLocationBeg);
+
+if (R.start.line == Pos.line && R.start.character <= Pos.character &&
+Pos.character <= R.end.character) {
+ IncludeTargets.push_back(Location{URIForFile{IncludeLoc.second},
+Range{Position{0, 0}, Position{0, 0}}});
+ return IncludeTargets;
+}
+ }
+
std::vector Decls = DeclMacrosFinder->takeDecls();
std::vector MacroInfos =
DeclMacrosFinder->takeMacroInfos();
@@ -242,13 +254,8 @@
DocumentHighlight getDocumentHighlight(SourceRange SR,
DocumentHighlightKind Kind) {
const SourceManager &S
[PATCH] D38639: [clangd] #include statements support for Open definition
ilya-biryukov added inline comments.
Comment at: clangd/ClangdUnit.h:51
+using IncludeReferenceMap = std::unordered_map;
+
We use `unordered_map` as a `vector>` here. (i.e. we never look up
values by key, because we don't only the range, merely a point in the range)
Replace map with `vector>` and remove `RangeHash` that we don't need
anymore.
Comment at: clangd/XRefs.cpp:185
+
+ if ((unsigned)R.start.line ==
+ SourceMgr.getSpellingLineNumber(
malaperle wrote:
> ilya-biryukov wrote:
> > malaperle wrote:
> > > ilya-biryukov wrote:
> > > > why do we need to convert to unsigned? To slience compiler warnings?
> > > Yes, "line" from the protocol is signed, whereas
> > > getSpellingColumn/lineNumber returns unsigned. I'll extract another var
> > > for the line number and cast both to int instead to have less casts and
> > > make the condition smaller.
> > Can we maybe convert to `clangd::Position` using the helper methods first
> > and do the comparison of two `clangd::Position`s?
> > Comparing between `clangd::Position` and clang's line/column numbers is a
> > common source of off-by-one errors in clangd.
> offsetToPosition (if that's what you mean) uses a StringRef of the code,
> which is not handy in this case. I'll add another one "sourceLocToPosition"
> to convert a SourceLocation to a Position. WDYT? It can be used in a few
> other places too.
Looks good, thanks!
Comment at: unittests/clangd/ClangdTests.cpp:781
+
+ Position P = Position{1, 11};
+
We moved `findDefinition` tests to `XRefsTests.cpp` and added a useful
`Annotation` helper to make writing these tests simpler.
Maybe we could use it for this test as well?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D38639
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38639: [clangd] #include statements support for Open definition
malaperle updated this revision to Diff 129293.
malaperle added a comment.
Revert an unintentional white space change.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D38639
Files:
clangd/ClangdUnit.cpp
clangd/ClangdUnit.h
clangd/Protocol.h
clangd/SourceCode.cpp
clangd/SourceCode.h
clangd/XRefs.cpp
unittests/clangd/ClangdTests.cpp
Index: unittests/clangd/ClangdTests.cpp
===
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -751,6 +751,74 @@
EXPECT_FALSE(PathResult.hasValue());
}
+TEST_F(ClangdVFSTest, CheckDefinitionIncludes) {
+ MockFSProvider FS;
+ ErrorCheckingDiagConsumer DiagConsumer;
+ MockCompilationDatabase CDB;
+
+ ClangdServer Server(CDB, DiagConsumer, FS, getDefaultAsyncThreadsCount(),
+ /*StorePreamblesInMemory=*/true);
+
+ auto FooCpp = getVirtualTestFilePath("foo.cpp");
+ const auto SourceContents = R"cpp(
+ #include "foo.h"
+ #include "invalid.h"
+ int b = a;
+ // test
+ int foo;
+ #include "foo.h"
+ )cpp";
+ FS.Files[FooCpp] = SourceContents;
+ auto FooH = getVirtualTestFilePath("foo.h");
+ const auto HeaderContents = "int a;";
+
+ FS.Files[FooCpp] = SourceContents;
+ FS.Files[FooH] = HeaderContents;
+
+ Server.addDocument(Context::empty(), FooH, HeaderContents);
+ Server.addDocument(Context::empty(), FooCpp, SourceContents);
+
+ Position P = Position{1, 11};
+
+ auto ExpectedLocations = Server.findDefinitions(Context::empty(), FooCpp, P);
+ ASSERT_TRUE(!!ExpectedLocations);
+ std::vector Locations = ExpectedLocations->Value;
+ EXPECT_TRUE(!Locations.empty());
+ std::string s("file:///");
+ std::string check = Locations[0].uri.uri;
+ check = check.erase(0, s.size() - 1);
+ check = check.substr(0, check.size());
+ ASSERT_EQ(check, FooH);
+ ASSERT_EQ(Locations[0].range.start.line, 0);
+ ASSERT_EQ(Locations[0].range.start.character, 0);
+ ASSERT_EQ(Locations[0].range.end.line, 0);
+ ASSERT_EQ(Locations[0].range.end.character, 0);
+
+ // Test include in preamble
+ Position P2 = Position{1, 11};
+
+ ExpectedLocations = Server.findDefinitions(Context::empty(), FooCpp, P2);
+ ASSERT_TRUE(!!ExpectedLocations);
+ Locations = ExpectedLocations->Value;
+ EXPECT_TRUE(!Locations.empty());
+
+ // Test invalid include
+ Position P3 = Position{2, 11};
+
+ ExpectedLocations = Server.findDefinitions(Context::empty(), FooCpp, P3);
+ ASSERT_TRUE(!!ExpectedLocations);
+ Locations = ExpectedLocations->Value;
+ EXPECT_TRUE(Locations.empty());
+
+ // Test include outside of Preamble
+ Position P4 = Position{6, 11};
+
+ ExpectedLocations = Server.findDefinitions(Context::empty(), FooCpp, P4);
+ ASSERT_TRUE(!!ExpectedLocations);
+ Locations = ExpectedLocations->Value;
+ EXPECT_TRUE(!Locations.empty());
+}
+
TEST_F(ClangdThreadingTest, NoConcurrentDiagnostics) {
class NoConcurrentAccessDiagConsumer : public DiagnosticsConsumer {
public:
Index: clangd/XRefs.cpp
===
--- clangd/XRefs.cpp
+++ clangd/XRefs.cpp
@@ -7,6 +7,7 @@
//
//===-===//
#include "XRefs.h"
+#include "SourceCode.h"
#include "clang/Index/IndexDataConsumer.h"
#include "clang/Index/IndexingAction.h"
namespace clang {
@@ -103,12 +104,8 @@
return llvm::None;
SourceLocation LocEnd = Lexer::getLocForEndOfToken(ValSourceRange.getEnd(), 0,
SourceMgr, LangOpts);
- Position Begin;
- Begin.line = SourceMgr.getSpellingLineNumber(LocStart) - 1;
- Begin.character = SourceMgr.getSpellingColumnNumber(LocStart) - 1;
- Position End;
- End.line = SourceMgr.getSpellingLineNumber(LocEnd) - 1;
- End.character = SourceMgr.getSpellingColumnNumber(LocEnd) - 1;
+ Position Begin = sourceLocToPosition(SourceMgr, LocStart);
+ Position End = sourceLocToPosition(SourceMgr, LocEnd);
Range R = {Begin, End};
Location L;
@@ -142,6 +139,21 @@
indexTopLevelDecls(AST.getASTContext(), AST.getTopLevelDecls(),
DeclMacrosFinder, IndexOpts);
+ /// Process targets for paths inside #include directive.
+ std::vector IncludeTargets;
+ for (auto &IncludeLoc : AST.getIRM()) {
+Range R = IncludeLoc.first;
+const SourceManager &SourceMgr = AST.getASTContext().getSourceManager();
+Position Pos = sourceLocToPosition(SourceMgr, SourceLocationBeg);
+
+if (R.start.line == Pos.line && R.start.character <= Pos.character &&
+Pos.character <= R.end.character) {
+ IncludeTargets.push_back(Location{URI::fromFile(IncludeLoc.second),
+Range{Position{0, 0}, Position{0, 0}}});
+ return IncludeTargets;
+}
+ }
+
std::vector Decls = DeclMacrosFinder->takeDecls();
std::vector MacroInfos =
DeclMacrosFinder->takeMacroInfos();
@@ -217,13 +229,8 @@
DocumentHighlight getDocumentHighligh
[PATCH] D38639: [clangd] #include statements support for Open definition
malaperle updated this revision to Diff 129292.
malaperle added a comment.
Store map in PremableData and copy it on reparse.
Convert SourceLoc to Position when comparing with include Positions.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D38639
Files:
clangd/ClangdUnit.cpp
clangd/ClangdUnit.h
clangd/Protocol.h
clangd/SourceCode.cpp
clangd/SourceCode.h
clangd/XRefs.cpp
unittests/clangd/ClangdTests.cpp
Index: unittests/clangd/ClangdTests.cpp
===
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -751,6 +751,74 @@
EXPECT_FALSE(PathResult.hasValue());
}
+TEST_F(ClangdVFSTest, CheckDefinitionIncludes) {
+ MockFSProvider FS;
+ ErrorCheckingDiagConsumer DiagConsumer;
+ MockCompilationDatabase CDB;
+
+ ClangdServer Server(CDB, DiagConsumer, FS, getDefaultAsyncThreadsCount(),
+ /*StorePreamblesInMemory=*/true);
+
+ auto FooCpp = getVirtualTestFilePath("foo.cpp");
+ const auto SourceContents = R"cpp(
+ #include "foo.h"
+ #include "invalid.h"
+ int b = a;
+ // test
+ int foo;
+ #include "foo.h"
+ )cpp";
+ FS.Files[FooCpp] = SourceContents;
+ auto FooH = getVirtualTestFilePath("foo.h");
+ const auto HeaderContents = "int a;";
+
+ FS.Files[FooCpp] = SourceContents;
+ FS.Files[FooH] = HeaderContents;
+
+ Server.addDocument(Context::empty(), FooH, HeaderContents);
+ Server.addDocument(Context::empty(), FooCpp, SourceContents);
+
+ Position P = Position{1, 11};
+
+ auto ExpectedLocations = Server.findDefinitions(Context::empty(), FooCpp, P);
+ ASSERT_TRUE(!!ExpectedLocations);
+ std::vector Locations = ExpectedLocations->Value;
+ EXPECT_TRUE(!Locations.empty());
+ std::string s("file:///");
+ std::string check = Locations[0].uri.uri;
+ check = check.erase(0, s.size() - 1);
+ check = check.substr(0, check.size());
+ ASSERT_EQ(check, FooH);
+ ASSERT_EQ(Locations[0].range.start.line, 0);
+ ASSERT_EQ(Locations[0].range.start.character, 0);
+ ASSERT_EQ(Locations[0].range.end.line, 0);
+ ASSERT_EQ(Locations[0].range.end.character, 0);
+
+ // Test include in preamble
+ Position P2 = Position{1, 11};
+
+ ExpectedLocations = Server.findDefinitions(Context::empty(), FooCpp, P2);
+ ASSERT_TRUE(!!ExpectedLocations);
+ Locations = ExpectedLocations->Value;
+ EXPECT_TRUE(!Locations.empty());
+
+ // Test invalid include
+ Position P3 = Position{2, 11};
+
+ ExpectedLocations = Server.findDefinitions(Context::empty(), FooCpp, P3);
+ ASSERT_TRUE(!!ExpectedLocations);
+ Locations = ExpectedLocations->Value;
+ EXPECT_TRUE(Locations.empty());
+
+ // Test include outside of Preamble
+ Position P4 = Position{6, 11};
+
+ ExpectedLocations = Server.findDefinitions(Context::empty(), FooCpp, P4);
+ ASSERT_TRUE(!!ExpectedLocations);
+ Locations = ExpectedLocations->Value;
+ EXPECT_TRUE(!Locations.empty());
+}
+
TEST_F(ClangdThreadingTest, NoConcurrentDiagnostics) {
class NoConcurrentAccessDiagConsumer : public DiagnosticsConsumer {
public:
Index: clangd/XRefs.cpp
===
--- clangd/XRefs.cpp
+++ clangd/XRefs.cpp
@@ -7,6 +7,7 @@
//
//===-===//
#include "XRefs.h"
+#include "SourceCode.h"
#include "clang/Index/IndexDataConsumer.h"
#include "clang/Index/IndexingAction.h"
namespace clang {
@@ -103,12 +104,8 @@
return llvm::None;
SourceLocation LocEnd = Lexer::getLocForEndOfToken(ValSourceRange.getEnd(), 0,
SourceMgr, LangOpts);
- Position Begin;
- Begin.line = SourceMgr.getSpellingLineNumber(LocStart) - 1;
- Begin.character = SourceMgr.getSpellingColumnNumber(LocStart) - 1;
- Position End;
- End.line = SourceMgr.getSpellingLineNumber(LocEnd) - 1;
- End.character = SourceMgr.getSpellingColumnNumber(LocEnd) - 1;
+ Position Begin = sourceLocToPosition(SourceMgr, LocStart);
+ Position End = sourceLocToPosition(SourceMgr, LocEnd);
Range R = {Begin, End};
Location L;
@@ -142,6 +139,21 @@
indexTopLevelDecls(AST.getASTContext(), AST.getTopLevelDecls(),
DeclMacrosFinder, IndexOpts);
+ /// Process targets for paths inside #include directive.
+ std::vector IncludeTargets;
+ for (auto &IncludeLoc : AST.getIRM()) {
+Range R = IncludeLoc.first;
+const SourceManager &SourceMgr = AST.getASTContext().getSourceManager();
+Position Pos = sourceLocToPosition(SourceMgr, SourceLocationBeg);
+
+if (R.start.line == Pos.line && R.start.character <= Pos.character &&
+Pos.character <= R.end.character) {
+ IncludeTargets.push_back(Location{URI::fromFile(IncludeLoc.second),
+Range{Position{0, 0}, Position{0, 0}}});
+ return IncludeTargets;
+}
+ }
+
std::vector Decls = DeclMacrosFinder->takeDecls();
std::vector MacroInfos =
DeclMacrosFinder->take
[PATCH] D38639: [clangd] #include statements support for Open definition
malaperle added inline comments.
Comment at: clangd/ClangdUnit.cpp:113
+ CppFilePreambleCallbacks(IncludeReferenceMap &IRM)
+ : IRM(IRM) {}
ilya-biryukov wrote:
> malaperle wrote:
> > ilya-biryukov wrote:
> > > ilya-biryukov wrote:
> > > > Let's create a new empty map inside this class and have a
> > > > `takeIncludeReferences` method (similar to `TopLevelDeclIDs` and
> > > > `takeTopLevelDeclIDs`)
> > > This comment is not addressed yet, but marked as done.
> > As mentioned below, the idea was to have a single map being appended to,
> > without having to merge two separate maps. However, I can change the code
> > so that two separate maps are built and merged if you prefer.
> >
> > But I'm not so clear if that's what you'd prefer:
> >
> > > You copy the map for preamble and then append to it in
> > > CppFilePreambleCallbacks? That also LG, we should not have many
> > > references there anyway.
> >
> > It's not meant to have any copy. The idea was to create a single
> > IncludeReferenceMap in CppFile::deferRebuild, populate it with both
> > preamble and non-preamble include references and std::move it around for
> > later use (stored in ParsedAST).
> We can't have a single map because AST is rebuilt more often than the
> Preamble, so we have two options:
> - Store a map for the preamble separately, copy it when we need to rebuild
> the AST and append references from the AST to the new instance of the map.
> - Store two maps: one contains references only from the Preamble, the other
> one from the AST.
>
> I think both are fine, since the copy of the map will be cheap anyway, as we
> only store a list of includes inside the main file.
> We can't have a single map because AST is rebuilt more often than the
> Preamble, so we have two options:
Doh! Indeed.
OK so I added the map in PreambleData, this one will say every reparse unless
the preamble changes. When the AST is built/rebuilt, the map is copied (or
empty) and includes from the AST are appended to it then stored in ParsedAST
(option #1?).
Comment at: clangd/XRefs.cpp:185
+
+ if ((unsigned)R.start.line ==
+ SourceMgr.getSpellingLineNumber(
ilya-biryukov wrote:
> malaperle wrote:
> > ilya-biryukov wrote:
> > > why do we need to convert to unsigned? To slience compiler warnings?
> > Yes, "line" from the protocol is signed, whereas
> > getSpellingColumn/lineNumber returns unsigned. I'll extract another var for
> > the line number and cast both to int instead to have less casts and make
> > the condition smaller.
> Can we maybe convert to `clangd::Position` using the helper methods first and
> do the comparison of two `clangd::Position`s?
> Comparing between `clangd::Position` and clang's line/column numbers is a
> common source of off-by-one errors in clangd.
offsetToPosition (if that's what you mean) uses a StringRef of the code, which
is not handy in this case. I'll add another one "sourceLocToPosition" to
convert a SourceLocation to a Position. WDYT? It can be used in a few other
places too.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D38639
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38639: [clangd] #include statements support for Open definition
ilya-biryukov added a comment.
@malaperle, hi! Both new diff and updating this works, so feel free the one
that suits you best. I tend to look over the whole change on each new round of
reviews anyway.
Comment at: clangd/ClangdUnit.cpp:113
+ CppFilePreambleCallbacks(IncludeReferenceMap &IRM)
+ : IRM(IRM) {}
malaperle wrote:
> ilya-biryukov wrote:
> > ilya-biryukov wrote:
> > > Let's create a new empty map inside this class and have a
> > > `takeIncludeReferences` method (similar to `TopLevelDeclIDs` and
> > > `takeTopLevelDeclIDs`)
> > This comment is not addressed yet, but marked as done.
> As mentioned below, the idea was to have a single map being appended to,
> without having to merge two separate maps. However, I can change the code so
> that two separate maps are built and merged if you prefer.
>
> But I'm not so clear if that's what you'd prefer:
>
> > You copy the map for preamble and then append to it in
> > CppFilePreambleCallbacks? That also LG, we should not have many references
> > there anyway.
>
> It's not meant to have any copy. The idea was to create a single
> IncludeReferenceMap in CppFile::deferRebuild, populate it with both preamble
> and non-preamble include references and std::move it around for later use
> (stored in ParsedAST).
We can't have a single map because AST is rebuilt more often than the Preamble,
so we have two options:
- Store a map for the preamble separately, copy it when we need to rebuild the
AST and append references from the AST to the new instance of the map.
- Store two maps: one contains references only from the Preamble, the other one
from the AST.
I think both are fine, since the copy of the map will be cheap anyway, as we
only store a list of includes inside the main file.
Comment at: clangd/ClangdUnit.cpp:281
+std::shared_ptr PCHs,
+IntrusiveRefCntPtr VFS, IncludeReferenceMap IRM) {
malaperle wrote:
> ilya-biryukov wrote:
> > Don't we already store the map we need in `PreambleData`? Why do we need an
> > extra `IRM` parameter?
> Since the map will be now stored in ParsedAST and the instance doesn't exist
> yet, we need to keep the IRM parameter. I noticed that it wasn't being
> std::move'd though.
That looks fine. Also see the other comment on why we need to copy the map from
the Preamble, and not `std::move` it.
Comment at: clangd/XRefs.cpp:185
+
+ if ((unsigned)R.start.line ==
+ SourceMgr.getSpellingLineNumber(
malaperle wrote:
> ilya-biryukov wrote:
> > why do we need to convert to unsigned? To slience compiler warnings?
> Yes, "line" from the protocol is signed, whereas getSpellingColumn/lineNumber
> returns unsigned. I'll extract another var for the line number and cast both
> to int instead to have less casts and make the condition smaller.
Can we maybe convert to `clangd::Position` using the helper methods first and
do the comparison of two `clangd::Position`s?
Comparing between `clangd::Position` and clang's line/column numbers is a
common source of off-by-one errors in clangd.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D38639
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38639: [clangd] #include statements support for Open definition
malaperle added a comment.
@ilya-biryukov Hi! I'll be updating William's patches that were in progress. I
just have a few comments/question before I send a new update. (I also don't
know if I can update this diff or I have to create a new diff on Phabricator...
I guess we'll see!!).
Comment at: clangd/ClangdUnit.cpp:113
+ CppFilePreambleCallbacks(IncludeReferenceMap &IRM)
+ : IRM(IRM) {}
ilya-biryukov wrote:
> ilya-biryukov wrote:
> > Let's create a new empty map inside this class and have a
> > `takeIncludeReferences` method (similar to `TopLevelDeclIDs` and
> > `takeTopLevelDeclIDs`)
> This comment is not addressed yet, but marked as done.
As mentioned below, the idea was to have a single map being appended to,
without having to merge two separate maps. However, I can change the code so
that two separate maps are built and merged if you prefer.
But I'm not so clear if that's what you'd prefer:
> You copy the map for preamble and then append to it in
> CppFilePreambleCallbacks? That also LG, we should not have many references
> there anyway.
It's not meant to have any copy. The idea was to create a single
IncludeReferenceMap in CppFile::deferRebuild, populate it with both preamble
and non-preamble include references and std::move it around for later use
(stored in ParsedAST).
Comment at: clangd/ClangdUnit.cpp:281
+std::shared_ptr PCHs,
+IntrusiveRefCntPtr VFS, IncludeReferenceMap IRM) {
ilya-biryukov wrote:
> Don't we already store the map we need in `PreambleData`? Why do we need an
> extra `IRM` parameter?
Since the map will be now stored in ParsedAST and the instance doesn't exist
yet, we need to keep the IRM parameter. I noticed that it wasn't being
std::move'd though.
Comment at: clangd/XRefs.cpp:185
+
+ if ((unsigned)R.start.line ==
+ SourceMgr.getSpellingLineNumber(
ilya-biryukov wrote:
> why do we need to convert to unsigned? To slience compiler warnings?
Yes, "line" from the protocol is signed, whereas getSpellingColumn/lineNumber
returns unsigned. I'll extract another var for the line number and cast both to
int instead to have less casts and make the condition smaller.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D38639
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38639: [clangd] #include statements support for Open definition
Nebiroth updated this revision to Diff 127900.
Nebiroth marked 11 inline comments as done.
Nebiroth added a comment.
Minor code cleanup
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D38639
Files:
clangd/ClangdUnit.cpp
clangd/ClangdUnit.h
clangd/Protocol.h
clangd/XRefs.cpp
unittests/clangd/ClangdTests.cpp
Index: unittests/clangd/ClangdTests.cpp
===
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -7,7 +7,6 @@
//
//===--===//
-#include "ClangdLSPServer.h"
#include "ClangdServer.h"
#include "Context.h"
#include "TestFS.h"
@@ -751,6 +750,75 @@
EXPECT_FALSE(PathResult.hasValue());
}
+TEST_F(ClangdVFSTest, CheckDefinitionIncludes) {
+ MockFSProvider FS;
+ ErrorCheckingDiagConsumer DiagConsumer;
+ MockCompilationDatabase CDB;
+
+ ClangdServer Server(CDB, DiagConsumer, FS, getDefaultAsyncThreadsCount(),
+ /*StorePreamblesInMemory=*/true,
+ EmptyLogger::getInstance());
+
+ auto FooCpp = getVirtualTestFilePath("foo.cpp");
+ const auto SourceContents = R"cpp(
+ #include "foo.h"
+ #include "invalid.h"
+ int b = a;
+ // test
+ int foo;
+ #include "foo.h"
+ )cpp";
+ FS.Files[FooCpp] = SourceContents;
+ auto FooH = getVirtualTestFilePath("foo.h");
+ const auto HeaderContents = "int a;";
+
+ FS.Files[FooCpp] = SourceContents;
+ FS.Files[FooH] = HeaderContents;
+
+ Server.addDocument(FooH, HeaderContents);
+ Server.addDocument(FooCpp, SourceContents);
+
+ Position P = Position{1, 11};
+
+ auto ExpectedLocations = Server.findDefinitions(FooCpp, P);
+ ASSERT_TRUE(!!ExpectedLocations);
+ std::vector Locations = ExpectedLocations->Value;
+ EXPECT_TRUE(!Locations.empty());
+ std::string s("file:///");
+ std::string check = Locations[0].uri.uri;
+ check = check.erase(0, s.size() - 1);
+ check = check.substr(0, check.size());
+ ASSERT_EQ(check, FooH);
+ ASSERT_EQ(Locations[0].range.start.line, 0);
+ ASSERT_EQ(Locations[0].range.start.character, 0);
+ ASSERT_EQ(Locations[0].range.end.line, 0);
+ ASSERT_EQ(Locations[0].range.end.character, 0);
+
+ // Test ctrl-clicking on the #include part on the statement
+ Position P2 = Position{1, 3};
+
+ ExpectedLocations = Server.findDefinitions(FooCpp, P2);
+ ASSERT_TRUE(!!ExpectedLocations);
+ Locations = ExpectedLocations->Value;
+ EXPECT_TRUE(!Locations.empty());
+
+ // Test invalid include
+ Position P3 = Position{2, 11};
+
+ ExpectedLocations = Server.findDefinitions(FooCpp, P3);
+ ASSERT_TRUE(!!ExpectedLocations);
+ Locations = ExpectedLocations->Value;
+ EXPECT_TRUE(Locations.empty());
+
+ // Test include outside of Preamble
+ Position P4 = Position{6, 5};
+
+ ExpectedLocations = Server.findDefinitions(FooCpp, P4);
+ ASSERT_TRUE(!!ExpectedLocations);
+ Locations = ExpectedLocations->Value;
+ EXPECT_TRUE(!Locations.empty());
+}
+
TEST_F(ClangdThreadingTest, NoConcurrentDiagnostics) {
class NoConcurrentAccessDiagConsumer : public DiagnosticsConsumer {
public:
Index: clangd/XRefs.cpp
===
--- clangd/XRefs.cpp
+++ clangd/XRefs.cpp
@@ -18,6 +18,7 @@
class DeclarationAndMacrosFinder : public index::IndexDataConsumer {
std::vector Decls;
std::vector MacroInfos;
+ std::vector DeclarationLocations;
const SourceLocation &SearchedLocation;
const ASTContext &AST;
Preprocessor &PP;
@@ -37,14 +38,26 @@
return std::move(Decls);
}
+ std::vector takeLocations() {
+// Don't keep the same location multiple times.
+// This can happen when nodes in the AST are visited twice.
+std::sort(DeclarationLocations.begin(), DeclarationLocations.end());
+auto Last =
+std::unique(DeclarationLocations.begin(), DeclarationLocations.end());
+DeclarationLocations.erase(Last, DeclarationLocations.end());
+return std::move(DeclarationLocations);
+ }
+
std::vector takeMacroInfos() {
// Don't keep the same Macro info multiple times.
std::sort(MacroInfos.begin(), MacroInfos.end());
auto Last = std::unique(MacroInfos.begin(), MacroInfos.end());
MacroInfos.erase(Last, MacroInfos.end());
return std::move(MacroInfos);
}
+ const SourceLocation &getSearchedLocation() { return SearchedLocation; };
+
bool
handleDeclOccurence(const Decl *D, index::SymbolRoleSet Roles,
ArrayRef Relations, FileID FID,
@@ -62,6 +75,25 @@
SourceMgr.getFileID(SearchedLocation) == FID;
}
+ void addDeclarationLocation(const SourceRange &ValSourceRange) {
+const SourceManager &SourceMgr = AST.getSourceManager();
+const LangOptions &LangOpts = AST.getLangOpts();
+SourceLocation LocStart = ValSourceRange.getBegin();
+SourceLocation LocEnd = Lexer::getLocForEndOfToken(ValSourceRange.getEnd(),
+ 0, SourceMgr, LangOpts);
+
[PATCH] D38639: [clangd] #include statements support for Open definition
ilya-biryukov added inline comments. Comment at: clangd/XRefs.cpp:183 + unsigned CharNumber = SourceMgr.getSpellingColumnNumber( + DeclMacrosFinder->getSearchedLocation()); + ilya-biryukov wrote: > Replace with `DeclMacrosFinder->getSearchedLocation()` `SourceLocationBeg`, > it makes the code easier to read. Sorry, the comment got messed up. Here's the correct version: Replace `DeclMacrosFinder->getSearchedLocation()` with `SourceLocationBeg`, it makes the code easier to read. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D38639 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38639: [clangd] #include statements support for Open definition
ilya-biryukov requested changes to this revision.
ilya-biryukov added a comment.
This revision now requires changes to proceed.
Another round of review
Comment at: clangd/ClangdUnit.cpp:113
+ CppFilePreambleCallbacks(IncludeReferenceMap &IRM)
+ : IRM(IRM) {}
ilya-biryukov wrote:
> Let's create a new empty map inside this class and have a
> `takeIncludeReferences` method (similar to `TopLevelDeclIDs` and
> `takeTopLevelDeclIDs`)
This comment is not addressed yet, but marked as done.
Comment at: clangd/ClangdUnit.cpp:147
+ IncludeReferenceMap &IRM;
+ std::vector>
TempPreambleIncludeLocations;
};
Nebiroth wrote:
> ilya-biryukov wrote:
> > We should have `SourceMgr` at all the proper places now, let's store
> > `IncludeReferenceMap` directly
> The idea was to pass a single map around to fill with every reference instead
> of having separate maps being merged together.
You copy the map for preamble and then append to it in
`CppFilePreambleCallbacks`? That also LG, we should not have many references
there anyway.
Comment at: clangd/ClangdUnit.cpp:281
+ IntrusiveRefCntPtr VFS,
+ IncludeReferenceMap IRM) {
Nebiroth wrote:
> ilya-biryukov wrote:
> > What reference does this `IncludeReferenceMap` contain? Includes from the
> > preamble?
> This should contain every reference available for one file.
Thanks for clarifying, LG.
Comment at: clangd/ClangdUnit.cpp:141
+ std::unique_ptr createPPCallbacks() {
+return llvm::make_unique(*SourceMgr, IRM);
+ }
Maybe add `assert(SourceMgr && "SourceMgr must be set at this point")` here?
Comment at: clangd/ClangdUnit.cpp:281
+std::shared_ptr PCHs,
+IntrusiveRefCntPtr VFS, IncludeReferenceMap IRM) {
Don't we already store the map we need in `PreambleData`? Why do we need an
extra `IRM` parameter?
Comment at: clangd/ClangdUnit.h:276
+std::vector
+findDefinitions(const Context &Ctx, ParsedAST &AST, Position Pos);
+
ilya-biryukov wrote:
> This function moved without need and lost a comment.
Now we have duplicated definitions here. Bad merge?
Comment at: clangd/CodeComplete.cpp:328
unsigned NumResults) override final {
-if (auto SS = Context.getCXXScopeSpecifier())
- CompletedName.SSInfo = extraCompletionScope(S, **SS);
+//if (auto SS = Context.getCXXScopeSpecifier())
+// CompletedName.SSInfo = extraCompletionScope(S, **SS);
Accidental change?
Comment at: clangd/CodeComplete.cpp:807
// Enable index-based code completion when Index is provided.
- Result.IncludeNamespaceLevelDecls = !Index;
+ // Result.IncludeNamespaceLevelDecls = !Index;
Accidental change?
Comment at: clangd/XRefs.cpp:41
+ std::vector takeLocations() {
+// Don't keep the same location multiple times.
We don't need locations anymore. Remove them.
Comment at: clangd/XRefs.cpp:177
+ std::vector IRMResult;
+ if (!AST.getIRM().empty()) {
Probably a good place for a comment. Also, maybe rename local var to something
easier to understand like `IncludeDefinitions`
```
/// Process targets for paths inside #include directive.
std::vector IncludeTargets;
```
Comment at: clangd/XRefs.cpp:178
+ std::vector IRMResult;
+ if (!AST.getIRM().empty()) {
+for (auto &IncludeLoc : AST.getIRM()) {
No need to special case empty maps, remove the if altogether.
Comment at: clangd/XRefs.cpp:183
+ unsigned CharNumber = SourceMgr.getSpellingColumnNumber(
+ DeclMacrosFinder->getSearchedLocation());
+
Replace with `DeclMacrosFinder->getSearchedLocation()` `SourceLocationBeg`, it
makes the code easier to read.
Comment at: clangd/XRefs.cpp:185
+
+ if ((unsigned)R.start.line ==
+ SourceMgr.getSpellingLineNumber(
why do we need to convert to unsigned? To slience compiler warnings?
Comment at: clangd/XRefs.cpp:188
+ DeclMacrosFinder->getSearchedLocation()) &&
+ ((unsigned)R.start.character >= CharNumber &&
+ CharNumber <= (unsigned)R.end.character)) {
this should be `R.start.charater <= CharNumber && CharNumber <= R.end.character`
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D38639
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38639: [clangd] #include statements support for Open definition
Nebiroth updated this revision to Diff 127617.
Nebiroth added a comment.
Removed some useless inclusions
Removed superfluous check when inserting data in map
Moved addition to DeclarationLocations in finish() outside of DeclMacrosFinder
Merged with revision 321087 (moved findDefinitions and findDocumentHighlights
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D38639
Files:
clangd/ClangdUnit.cpp
clangd/ClangdUnit.h
clangd/CodeComplete.cpp
clangd/Protocol.h
clangd/XRefs.cpp
unittests/clangd/ClangdTests.cpp
Index: unittests/clangd/ClangdTests.cpp
===
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -7,7 +7,6 @@
//
//===--===//
-#include "ClangdLSPServer.h"
#include "ClangdServer.h"
#include "Context.h"
#include "TestFS.h"
@@ -751,6 +750,75 @@
EXPECT_FALSE(PathResult.hasValue());
}
+TEST_F(ClangdVFSTest, CheckDefinitionIncludes) {
+ MockFSProvider FS;
+ ErrorCheckingDiagConsumer DiagConsumer;
+ MockCompilationDatabase CDB;
+
+ ClangdServer Server(CDB, DiagConsumer, FS, getDefaultAsyncThreadsCount(),
+ /*StorePreamblesInMemory=*/true,
+ EmptyLogger::getInstance());
+
+ auto FooCpp = getVirtualTestFilePath("foo.cpp");
+ const auto SourceContents = R"cpp(
+ #include "foo.h"
+ #include "invalid.h"
+ int b = a;
+ // test
+ int foo;
+ #include "foo.h"
+ )cpp";
+ FS.Files[FooCpp] = SourceContents;
+ auto FooH = getVirtualTestFilePath("foo.h");
+ const auto HeaderContents = "int a;";
+
+ FS.Files[FooCpp] = SourceContents;
+ FS.Files[FooH] = HeaderContents;
+
+ Server.addDocument(FooH, HeaderContents);
+ Server.addDocument(FooCpp, SourceContents);
+
+ Position P = Position{1, 11};
+
+ auto ExpectedLocations = Server.findDefinitions(FooCpp, P);
+ ASSERT_TRUE(!!ExpectedLocations);
+ std::vector Locations = ExpectedLocations->Value;
+ EXPECT_TRUE(!Locations.empty());
+ std::string s("file:///");
+ std::string check = Locations[0].uri.uri;
+ check = check.erase(0, s.size() - 1);
+ check = check.substr(0, check.size());
+ ASSERT_EQ(check, FooH);
+ ASSERT_EQ(Locations[0].range.start.line, 0);
+ ASSERT_EQ(Locations[0].range.start.character, 0);
+ ASSERT_EQ(Locations[0].range.end.line, 0);
+ ASSERT_EQ(Locations[0].range.end.character, 0);
+
+ // Test ctrl-clicking on the #include part on the statement
+ Position P2 = Position{1, 3};
+
+ ExpectedLocations = Server.findDefinitions(FooCpp, P2);
+ ASSERT_TRUE(!!ExpectedLocations);
+ Locations = ExpectedLocations->Value;
+ EXPECT_TRUE(!Locations.empty());
+
+ // Test invalid include
+ Position P3 = Position{2, 11};
+
+ ExpectedLocations = Server.findDefinitions(FooCpp, P3);
+ ASSERT_TRUE(!!ExpectedLocations);
+ Locations = ExpectedLocations->Value;
+ EXPECT_TRUE(Locations.empty());
+
+ // Test include outside of Preamble
+ Position P4 = Position{6, 5};
+
+ ExpectedLocations = Server.findDefinitions(FooCpp, P4);
+ ASSERT_TRUE(!!ExpectedLocations);
+ Locations = ExpectedLocations->Value;
+ EXPECT_TRUE(!Locations.empty());
+}
+
TEST_F(ClangdThreadingTest, NoConcurrentDiagnostics) {
class NoConcurrentAccessDiagConsumer : public DiagnosticsConsumer {
public:
Index: clangd/XRefs.cpp
===
--- clangd/XRefs.cpp
+++ clangd/XRefs.cpp
@@ -18,6 +18,7 @@
class DeclarationAndMacrosFinder : public index::IndexDataConsumer {
std::vector Decls;
std::vector MacroInfos;
+ std::vector DeclarationLocations;
const SourceLocation &SearchedLocation;
const ASTContext &AST;
Preprocessor &PP;
@@ -37,14 +38,26 @@
return std::move(Decls);
}
+ std::vector takeLocations() {
+// Don't keep the same location multiple times.
+// This can happen when nodes in the AST are visited twice.
+std::sort(DeclarationLocations.begin(), DeclarationLocations.end());
+auto Last =
+std::unique(DeclarationLocations.begin(), DeclarationLocations.end());
+DeclarationLocations.erase(Last, DeclarationLocations.end());
+return std::move(DeclarationLocations);
+ }
+
std::vector takeMacroInfos() {
// Don't keep the same Macro info multiple times.
std::sort(MacroInfos.begin(), MacroInfos.end());
auto Last = std::unique(MacroInfos.begin(), MacroInfos.end());
MacroInfos.erase(Last, MacroInfos.end());
return std::move(MacroInfos);
}
+ const SourceLocation &getSearchedLocation() { return SearchedLocation; };
+
bool
handleDeclOccurence(const Decl *D, index::SymbolRoleSet Roles,
ArrayRef Relations, FileID FID,
@@ -62,6 +75,25 @@
SourceMgr.getFileID(SearchedLocation) == FID;
}
+ void addDeclarationLocation(const SourceRange &ValSourceRange) {
+const SourceManager &SourceMgr = AST.getSourceManager();
+const LangOptions &LangOpts = AST.getLangOpts();
+Sour
[PATCH] D38639: [clangd] #include statements support for Open definition
Nebiroth marked 8 inline comments as done.
Nebiroth added inline comments.
Comment at: clangd/ClangdUnit.cpp:85
+
+if (SourceMgr.getMainFileID() ==
SourceMgr.getFileID(FilenameRange.getAsRange().getBegin()) &&
SourceMgr.isInMainFile(FilenameRange.getAsRange().getBegin())) {
+ // Only inclusion directives in the main file make sense. The user cannot
ilya-biryukov wrote:
> ilya-biryukov wrote:
> > `clang-format` please
> Do we need both checks? Doesn't `SourceMgr.isInMainFile` handles all the
> cases?
This check was here to prevent an issue that appeared previously. I don't think
this check is needed anymore.
Comment at: clangd/ClangdUnit.cpp:147
+ IncludeReferenceMap &IRM;
+ std::vector>
TempPreambleIncludeLocations;
};
ilya-biryukov wrote:
> We should have `SourceMgr` at all the proper places now, let's store
> `IncludeReferenceMap` directly
The idea was to pass a single map around to fill with every reference instead
of having separate maps being merged together.
Comment at: clangd/ClangdUnit.cpp:281
+ IntrusiveRefCntPtr VFS,
+ IncludeReferenceMap IRM) {
ilya-biryukov wrote:
> What reference does this `IncludeReferenceMap` contain? Includes from the
> preamble?
This should contain every reference available for one file.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D38639
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38639: [clangd] #include statements support for Open definition
ilya-biryukov requested changes to this revision.
ilya-biryukov added a comment.
This revision now requires changes to proceed.
Thanks for addressing the comments quickly.
I took another look and added a few more comments.
This moves in the right direction, though, this is really close to landing.
Comment at: clangd/ClangdServer.cpp:25
#include
+#include
Is this include redundant now? Can we remove it?
Comment at: clangd/ClangdServer.cpp:430
+ Resources->getAST().get()->runUnderLock([Pos, &Result, &Ctx,
+ this](ParsedAST *AST) {
if (!AST)
Capturing `this` seems redundant. Remove it?
Comment at: clangd/ClangdUnit.cpp:85
+
+if (SourceMgr.getMainFileID() ==
SourceMgr.getFileID(FilenameRange.getAsRange().getBegin()) &&
SourceMgr.isInMainFile(FilenameRange.getAsRange().getBegin())) {
+ // Only inclusion directives in the main file make sense. The user cannot
`clang-format` please
Comment at: clangd/ClangdUnit.cpp:85
+
+if (SourceMgr.getMainFileID() ==
SourceMgr.getFileID(FilenameRange.getAsRange().getBegin()) &&
SourceMgr.isInMainFile(FilenameRange.getAsRange().getBegin())) {
+ // Only inclusion directives in the main file make sense. The user cannot
ilya-biryukov wrote:
> `clang-format` please
Do we need both checks? Doesn't `SourceMgr.isInMainFile` handles all the cases?
Comment at: clangd/ClangdUnit.cpp:113
+ CppFilePreambleCallbacks(IncludeReferenceMap &IRM)
+ : IRM(IRM) {}
Let's create a new empty map inside this class and have a
`takeIncludeReferences` method (similar to `TopLevelDeclIDs` and
`takeTopLevelDeclIDs`)
Comment at: clangd/ClangdUnit.cpp:147
+ IncludeReferenceMap &IRM;
+ std::vector>
TempPreambleIncludeLocations;
};
We should have `SourceMgr` at all the proper places now, let's store
`IncludeReferenceMap` directly
Comment at: clangd/ClangdUnit.cpp:281
+ IntrusiveRefCntPtr VFS,
+ IncludeReferenceMap IRM) {
What reference does this `IncludeReferenceMap` contain? Includes from the
preamble?
Comment at: clangd/ClangdUnit.cpp:347
const SourceLocation &SearchedLocation,
- ASTContext &AST, Preprocessor &PP)
- : SearchedLocation(SearchedLocation), AST(AST), PP(PP) {}
+ ASTContext &AST, Preprocessor &PP, const
IncludeReferenceMap &IncludeLocationMap)
+ : SearchedLocation(SearchedLocation), AST(AST), PP(PP),
IncludeLocationMap(IncludeLocationMap) {}
This class handles processing AST and preprocessor, it does not need to get
`IncludeLocationMap` in constructor or store it at all.
Remove `IncludeLocationMap` from this class and handle getting references from
`IncludeLocationMap` outside this class instead.
Comment at: clangd/ClangdUnit.h:97
+ IncludeReferenceMap &getIRM() { return IRM; };
+
Make it all const?
`const IncludeReferenceMap &getIRM() const { return IRM; }`
Comment at: clangd/ClangdUnit.h:276
+std::vector
+findDefinitions(const Context &Ctx, ParsedAST &AST, Position Pos);
+
This function moved without need and lost a comment.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D38639
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38639: [clangd] #include statements support for Open definition
Nebiroth updated this revision to Diff 127386.
Nebiroth marked 14 inline comments as done.
Nebiroth added a comment.
Herald added a subscriber: mgrang.
CppFilePreambleCallbacks no longer extends PPCallbacks
CppFilePreambleCallbacks no longer needs SourceManager parameter
Removed temporary vector TempPreambleIncludeLocations
IncludeReferenceMap in ParsedAST no longer passed by reference
Code handling includes outside of preambles is now separated in
IncludeRefsCollector class
Removed addLocation
Changed isSameLine function name and now checks if searched location is in
range instead of verifying the line
Removed changes to findDefinitions interface
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D38639
Files:
clangd/ClangdServer.cpp
clangd/ClangdUnit.cpp
clangd/ClangdUnit.h
clangd/Protocol.h
unittests/clangd/ClangdTests.cpp
Index: unittests/clangd/ClangdTests.cpp
===
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -7,7 +7,6 @@
//
//===--===//
-#include "ClangdLSPServer.h"
#include "ClangdServer.h"
#include "Context.h"
#include "TestFS.h"
@@ -751,6 +750,75 @@
EXPECT_FALSE(PathResult.hasValue());
}
+TEST_F(ClangdVFSTest, CheckDefinitionIncludes) {
+ MockFSProvider FS;
+ ErrorCheckingDiagConsumer DiagConsumer;
+ MockCompilationDatabase CDB;
+
+ ClangdServer Server(CDB, DiagConsumer, FS, getDefaultAsyncThreadsCount(),
+ /*StorePreamblesInMemory=*/true,
+ EmptyLogger::getInstance());
+
+ auto FooCpp = getVirtualTestFilePath("foo.cpp");
+ const auto SourceContents = R"cpp(
+ #include "foo.h"
+ #include "invalid.h"
+ int b = a;
+ // test
+ int foo;
+ #include "foo.h"
+ )cpp";
+ FS.Files[FooCpp] = SourceContents;
+ auto FooH = getVirtualTestFilePath("foo.h");
+ const auto HeaderContents = "int a;";
+
+ FS.Files[FooCpp] = SourceContents;
+ FS.Files[FooH] = HeaderContents;
+
+ Server.addDocument(FooH, HeaderContents);
+ Server.addDocument(FooCpp, SourceContents);
+
+ Position P = Position{1, 11};
+
+ auto ExpectedLocations = Server.findDefinitions(FooCpp, P);
+ ASSERT_TRUE(!!ExpectedLocations);
+ std::vector Locations = ExpectedLocations->Value;
+ EXPECT_TRUE(!Locations.empty());
+ std::string s("file:///");
+ std::string check = Locations[0].uri.uri;
+ check = check.erase(0, s.size() - 1);
+ check = check.substr(0, check.size());
+ ASSERT_EQ(check, FooH);
+ ASSERT_EQ(Locations[0].range.start.line, 0);
+ ASSERT_EQ(Locations[0].range.start.character, 0);
+ ASSERT_EQ(Locations[0].range.end.line, 0);
+ ASSERT_EQ(Locations[0].range.end.character, 0);
+
+ // Test ctrl-clicking on the #include part on the statement
+ Position P2 = Position{1, 3};
+
+ ExpectedLocations = Server.findDefinitions(FooCpp, P2);
+ ASSERT_TRUE(!!ExpectedLocations);
+ Locations = ExpectedLocations->Value;
+ EXPECT_TRUE(!Locations.empty());
+
+ // Test invalid include
+ Position P3 = Position{2, 11};
+
+ ExpectedLocations = Server.findDefinitions(FooCpp, P3);
+ ASSERT_TRUE(!!ExpectedLocations);
+ Locations = ExpectedLocations->Value;
+ EXPECT_TRUE(Locations.empty());
+
+ // Test include outside of Preamble
+ Position P4 = Position{6, 5};
+
+ ExpectedLocations = Server.findDefinitions(FooCpp, P4);
+ ASSERT_TRUE(!!ExpectedLocations);
+ Locations = ExpectedLocations->Value;
+ EXPECT_TRUE(!Locations.empty());
+}
+
TEST_F(ClangdThreadingTest, NoConcurrentDiagnostics) {
class NoConcurrentAccessDiagConsumer : public DiagnosticsConsumer {
public:
Index: clangd/Protocol.h
===
--- clangd/Protocol.h
+++ clangd/Protocol.h
@@ -108,6 +108,14 @@
bool fromJSON(const json::Expr &, Range &);
json::Expr toJSON(const Range &);
+class RangeHash {
+public:
+ std::size_t operator()(const Range &R) const {
+return ((R.start.line & 0x18) << 3) | ((R.start.character & 0x18) << 1) |
+ ((R.end.line & 0x18) >> 1) | ((R.end.character & 0x18) >> 3);
+ }
+};
+
struct Location {
/// The text document's URI.
URI uri;
Index: clangd/ClangdUnit.h
===
--- clangd/ClangdUnit.h
+++ clangd/ClangdUnit.h
@@ -23,6 +23,7 @@
#include
#include
#include
+#include
namespace llvm {
class raw_ostream;
@@ -58,17 +59,22 @@
std::vector Diags;
};
+using IncludeReferenceMap = std::unordered_map;
+
/// Stores and provides access to parsed AST.
class ParsedAST {
public:
/// Attempts to run Clang and store parsed AST. If \p Preamble is non-null
/// it is reused during parsing.
static llvm::Optional
- Build(const Context &Ctx, std::unique_ptr CI,
-std::shared_ptr Preamble,
-std::unique_ptr Buffer,
-std::shared_ptr PCHs,
-IntrusiveRefCntPtr VFS);
+ Build(const Context &Ctx,
+ std::unique_ptr
[PATCH] D38639: [clangd] #include statements support for Open definition
ilya-biryukov requested changes to this revision.
ilya-biryukov added inline comments.
This revision now requires changes to proceed.
Comment at: clangd/ClangdServer.cpp:446
std::vector Result;
- Resources->getAST().get()->runUnderLock([Pos, &Result, this](ParsedAST *AST)
{
+ std::shared_future> Preamble =
+ Resources->getPreamble();
We don't need this `Preamble` variable, right?
Comment at: clangd/ClangdUnit.cpp:73
-class CppFilePreambleCallbacks : public PreambleCallbacks {
+class CppFilePreambleCallbacks : public PreambleCallbacks, public PPCallbacks {
public:
We don't need to derive this class from `PPCallbacks`, `DelegatingPPCallbacks`
can call any method on it anyway as it knows the exact type. Please remove this
inheritance.
Comment at: clangd/ClangdUnit.cpp:79
+ CppFilePreambleCallbacks(SourceManager *SourceMgr, IncludeReferenceMap &IRM)
+ : SourceMgr(SourceMgr), IRM(IRM) {}
Instead of passing `SourceManager` in constructor, maybe add a
`BeforeExecute(CompilerInstance &CI)` method to `PreambleCallbacks` and set
`SourceManager` when its called.
Comment at: clangd/ClangdUnit.cpp:167
+llvm::make_unique(*this);
+return DelegatedPPCallbacks;
+ }
NIT: replace with `return llvm::make_unique(*this);` and
remove the local variable.
Comment at: clangd/ClangdUnit.cpp:175
+ IncludeReferenceMap &IRM;
+ std::vector>
TempPreambleIncludeLocations;
};
Adding `BeforeExecute` would allow to get rid of this vector.
Comment at: clangd/ClangdUnit.cpp:247
IntrusiveRefCntPtr VFS,
- clangd::Logger &Logger) {
+ clangd::Logger &Logger, IncludeReferenceMap &IRM) {
Let's store `IncludeReferenceMap` in `ParsedAST` instead of having it as out
parameter.
It allows to properly manage the lifetime of `IncludeReferenceMap` (logically,
it is exactly tied to the AST).
Comment at: clangd/ClangdUnit.cpp:270
+
+ CppFilePreambleCallbacks SerializedDeclsCollector(&Clang->getSourceManager(),
+IRM);
Why do we use `CppFilePreambleCallbacks` here? Factor the code that handles the
references into a separate class and use it here:
```
class IncludeRefsCollector : public PPCallbacks {
public:
IncludeRefsCollector(IncludeReferenceMap &Refs) : Refs(Refs) {}
// implementation of PPCallbacks ...
private;
IncludeReferenceMap &Refs;
};
/// .
class CppFilePreambleCaallbacks {
public:
//
std::unique_ptr createPPCallbacks() { return
llvm::make_unique(this->IRM); }
};
//
void ParsedAST::Build() {
//
// No need to create CppFilePreambleCallbacks here.
Clang->getPreprocessor().addPPCallbacks(llvm::make_unique(IRM));
//
}
```
Comment at: clangd/ClangdUnit.cpp:309
Preprocessor &PP;
+ std::unordered_map IncludeLocationMap;
Use `IncludeReferenceMap` typedef here.
Comment at: clangd/ClangdUnit.cpp:315
+ Preprocessor &PP,
+ std::unordered_map IncludeLocationMap)
+ : SearchedLocation(SearchedLocation), AST(AST), PP(PP),
- use a typedef for the map here
- accept the map by const reference to avoid copies
Comment at: clangd/ClangdUnit.cpp:365
Range R = {Begin, End};
+addLocation(URI::fromFile(
+SourceMgr.getFilename(SourceMgr.getSpellingLoc(LocStart))),
Do we change semantics of getting the file paths here? Maybe leave the code as
is?
Comment at: clangd/ClangdUnit.cpp:370
+
+ void addLocation(URI Uri, Range R) {
Location L;
This method does not seem particularly useful. One can construct location like
this: `Location{Uri, R}`, the whole method can be written as
`DeclarationLocations.push_back(Location{Uri, R})`.
Comment at: clangd/ClangdUnit.cpp:378
void finish() override {
+
+for (auto &IncludeLoc : IncludeLocationMap) {
NIT: empty linke at the start of the function
Comment at: clangd/ClangdUnit.cpp:381
+ Range R = IncludeLoc.first;
+ if (isSameLine(R.start.line)) {
+addLocation(URI::fromFile(IncludeLoc.second), Range{Position{0,0},
Position{0,0}});
Let's check the the point is in range, not that it's not the same line. So that
we don't trigger goto in strange places, like comments:
```
#include // this include is very important. <--- we should not skip
to include when editor caret is over a comment
```
Comment at: clangd/ClangdUnit.cpp:420
+ParsedAST &AST, Position Pos, clangd::Logger &Logger,
+std::unordered_map IncludeLocationMap) {
const SourceManager &SourceMgr = AST.getASTContext().getSo
[PATCH] D38639: [clangd] #include statements support for Open definition
Nebiroth updated this revision to Diff 127161.
Nebiroth marked 27 inline comments as done.
Nebiroth added a comment.
inner class IncludeReferenceMap replaced by one map
fillRangeVector() and findPreambleIncludes() code moved into wrapper class
Open definition now returns an empty Range struct (line, character = 0)
Fixed unit tests
Minor code improvements
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D38639
Files:
clangd/ClangdServer.cpp
clangd/ClangdUnit.cpp
clangd/ClangdUnit.h
clangd/Protocol.h
unittests/clangd/ClangdTests.cpp
Index: unittests/clangd/ClangdTests.cpp
===
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -7,7 +7,6 @@
//
//===--===//
-#include "ClangdLSPServer.h"
#include "ClangdServer.h"
#include "Logger.h"
#include "TestFS.h"
@@ -749,6 +748,75 @@
EXPECT_FALSE(PathResult.hasValue());
}
+TEST_F(ClangdVFSTest, CheckDefinitionIncludes) {
+ MockFSProvider FS;
+ ErrorCheckingDiagConsumer DiagConsumer;
+ MockCompilationDatabase CDB;
+
+ ClangdServer Server(CDB, DiagConsumer, FS, getDefaultAsyncThreadsCount(),
+ /*StorePreamblesInMemory=*/true,
+ EmptyLogger::getInstance());
+
+ auto FooCpp = getVirtualTestFilePath("foo.cpp");
+ const auto SourceContents = R"cpp(
+ #include "foo.h"
+ #include "invalid.h"
+ int b = a;
+ // test
+ int foo;
+ #include "foo.h"
+ )cpp";
+ FS.Files[FooCpp] = SourceContents;
+ auto FooH = getVirtualTestFilePath("foo.h");
+ const auto HeaderContents = "int a;";
+
+ FS.Files[FooCpp] = SourceContents;
+ FS.Files[FooH] = HeaderContents;
+
+ Server.addDocument(FooH, HeaderContents);
+ Server.addDocument(FooCpp, SourceContents);
+
+ Position P = Position{1, 11};
+
+ auto ExpectedLocations = Server.findDefinitions(FooCpp, P);
+ ASSERT_TRUE(!!ExpectedLocations);
+ std::vector Locations = ExpectedLocations->Value;
+ EXPECT_TRUE(!Locations.empty());
+ std::string s("file:///");
+ std::string check = Locations[0].uri.uri;
+ check = check.erase(0, s.size() - 1);
+ check = check.substr(0, check.size());
+ ASSERT_EQ(check, FooH);
+ ASSERT_EQ(Locations[0].range.start.line, 0);
+ ASSERT_EQ(Locations[0].range.start.character, 0);
+ ASSERT_EQ(Locations[0].range.end.line, 0);
+ ASSERT_EQ(Locations[0].range.end.character, 0);
+
+ // Test ctrl-clicking on the #include part on the statement
+ Position P2 = Position{1, 3};
+
+ ExpectedLocations = Server.findDefinitions(FooCpp, P2);
+ ASSERT_TRUE(!!ExpectedLocations);
+ Locations = ExpectedLocations->Value;
+ EXPECT_TRUE(!Locations.empty());
+
+ // Test invalid include
+ Position P3 = Position{2, 11};
+
+ ExpectedLocations = Server.findDefinitions(FooCpp, P3);
+ ASSERT_TRUE(!!ExpectedLocations);
+ Locations = ExpectedLocations->Value;
+ EXPECT_TRUE(Locations.empty());
+
+ // Test include outside of Preamble
+ Position P4 = Position{6, 5};
+
+ ExpectedLocations = Server.findDefinitions(FooCpp, P4);
+ ASSERT_TRUE(!!ExpectedLocations);
+ Locations = ExpectedLocations->Value;
+ EXPECT_TRUE(!Locations.empty());
+}
+
TEST_F(ClangdThreadingTest, NoConcurrentDiagnostics) {
class NoConcurrentAccessDiagConsumer : public DiagnosticsConsumer {
public:
Index: clangd/Protocol.h
===
--- clangd/Protocol.h
+++ clangd/Protocol.h
@@ -108,6 +108,14 @@
bool fromJSON(const json::Expr &, Range &);
json::Expr toJSON(const Range &);
+class RangeHash {
+public:
+ std::size_t operator()(const Range &R) const {
+return ((R.start.line & 0x18) << 3) | ((R.start.character & 0x18) << 1) |
+ ((R.end.line & 0x18) >> 1) | ((R.end.character & 0x18) >> 3);
+ }
+};
+
struct Location {
/// The text document's URI.
URI uri;
Index: clangd/ClangdUnit.h
===
--- clangd/ClangdUnit.h
+++ clangd/ClangdUnit.h
@@ -22,6 +22,7 @@
#include
#include
#include
+#include
namespace llvm {
class raw_ostream;
@@ -59,6 +60,8 @@
std::vector Diags;
};
+using IncludeReferenceMap = std::unordered_map;
+
/// Stores and provides access to parsed AST.
class ParsedAST {
public:
@@ -69,7 +72,8 @@
std::shared_ptr Preamble,
std::unique_ptr Buffer,
std::shared_ptr PCHs,
-IntrusiveRefCntPtr VFS, clangd::Logger &Logger);
+IntrusiveRefCntPtr VFS, clangd::Logger &Logger,
+IncludeReferenceMap &IRM);
ParsedAST(ParsedAST &&Other);
ParsedAST &operator=(ParsedAST &&Other);
@@ -89,12 +93,14 @@
const std::vector &getDiagnostics() const;
+ IncludeReferenceMap &getIRM() { return IRM; };
+
private:
ParsedAST(std::shared_ptr Preamble,
std::unique_ptr Clang,
std::unique_ptr Action,
std::vector TopLevelDecls,
-std::vector Diags);
+std::vector Diags, IncludeRef
[PATCH] D38639: [clangd] #include statements support for Open definition
malaperle requested changes to this revision.
malaperle added inline comments.
This revision now requires changes to proceed.
Comment at: clangd/ClangdServer.cpp:454
+
+IncludeReferenceMap IRM = std::move(AST->takeIRM());
+Result = clangd::findDefinitions(*AST, Pos, Logger,
IRM.IncludeLocationMap);
IncludeReferenceMap & here? See other comment in takeIRM
Comment at: clangd/ClangdUnit.cpp:34
#include
+#include
remove
Comment at: clangd/ClangdUnit.cpp:77
-class CppFilePreambleCallbacks : public PreambleCallbacks {
+void fillRangeVector(
+const SourceManager &SM,
remove
Comment at: clangd/ClangdUnit.cpp:96
+
+void findPreambleIncludes(
+const SourceManager &SM,
remove
Comment at: clangd/ClangdUnit.cpp:118
+ CppFilePreambleCallbacks(SourceManager *SourceMgr, IncludeReferenceMap &IRM)
+ : SourceMgr(SourceMgr), IRM(IRM) {}
+
These need to be swapped to be in the same order that they are declared below.
Comment at: clangd/ClangdUnit.cpp:120
+
+ IncludeReferenceMap takeIRM() {
+fillRangeVector(*SourceMgr, IRM.DataVector, IRM.RangeVector);
I don't think we need this if we pass the map by reference (and store it as a
reference, see other comment)
Comment at: clangd/ClangdUnit.cpp:127
+
+ IncludeReferenceMap getIRM() { return IRM; };
+
Remove (see previous comment)
Comment at: clangd/ClangdUnit.cpp:155
+ }
+
+ void InclusionDirective(SourceLocation HashLoc, const Token &IncludeTok,
I tried to simply the methods introduced here:
```
void AfterExecute(CompilerInstance &CI) override {
SourceMgr = &CI.getSourceManager();
for (auto InclusionLoc : TempPreambleIncludeLocations)
addIncludeLocation(InclusionLoc);
}
void InclusionDirective(SourceLocation HashLoc, const Token &IncludeTok,
StringRef FileName, bool IsAngled,
CharSourceRange FilenameRange, const FileEntry *File,
StringRef SearchPath, StringRef RelativePath,
const Module *Imported) override {
auto SR = FilenameRange.getAsRange();
if (SR.isInvalid() || !File || File->tryGetRealPathName().empty())
return;
if (SourceMgr) {
addIncludeLocation({SR, File->tryGetRealPathName()});
} else {
// When we are processing the inclusion directives inside the preamble,
// we don't have access to a SourceManager, so we cannot convert
// SourceRange to Range. This is done instead in AfterExecute when a
// SourceManager becomes available.
TempPreambleIncludeLocations.push_back({SR, File->tryGetRealPathName()});
}
}
void addIncludeLocation(std::pair InclusionLoc) {
// Only inclusion directives in the main file make sense. The user cannot
// select directives not in the main file.
if (SourceMgr->getMainFileID() ==
SourceMgr->getFileID(InclusionLoc.first.getBegin()))
IRM.insert({getRange(InclusionLoc.first), InclusionLoc.second});
}
Range getRange(SourceRange SR) {
Position Begin;
Begin.line = SourceMgr->getSpellingLineNumber(SR.getBegin());
Begin.character = SourceMgr->getSpellingColumnNumber(SR.getBegin());
Position End;
End.line = SourceMgr->getSpellingLineNumber(SR.getEnd());
End.character = SourceMgr->getSpellingColumnNumber(SR.getEnd());
return {Begin, End};
}
```
Comment at: clangd/ClangdUnit.cpp:177
+ Range R = {Begin, End};
+ if (File && !File->tryGetRealPathName().empty())
+IRM.IncludeLocationMap.insert({R, File->tryGetRealPathName()});
I think we need a "is main file" check here. In case this is used on a AST with
no preamble.
Comment at: clangd/ClangdUnit.cpp:209
std::vector TopLevelDeclIDs;
+ IncludeReferenceMap IRM;
+ SourceManager *SourceMgr;
IncludeReferenceMap &IRM;
That way, we can use the same map and pass it multiple times to different
"collectors".
Comment at: clangd/ClangdUnit.cpp:266
+class EmptyDiagsConsumer : public DiagnosticConsumer {
+public:
I don't think this change was brought back intentionally?
Comment at: clangd/ClangdUnit.cpp:288
IntrusiveRefCntPtr VFS,
- clangd::Logger &Logger) {
+ clangd::Logger &Logger, IncludeReferenceMap IRM) {
IncludeReferenceMap &IRM
Comment at: clangd/ClangdUnit.cpp:314
+
+
Clang->getPreprocessor().addPPCallbacks(std::move(SerializedDeclsCollector.createPPCallbacks()));
+
unnecessary std::move
Comment at: clangd/ClangdUnit.cpp:325
+ std::move(ParsedDecls), std::move(ASTDi
[PATCH] D38639: [clangd] #include statements support for Open definition
Nebiroth updated this revision to Diff 126429.
Nebiroth added a comment.
Creating unique_ptr for wrapper class now uses overriden method
createPPCallbacks() from PrecompiledPreamble class
Moved wrapper class to inline inside createPPCallbacks()
Wrapper class now uses CppFilePreambleCallbacks as field
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D38639
Files:
clangd/ClangdServer.cpp
clangd/ClangdUnit.cpp
clangd/ClangdUnit.h
clangd/Protocol.h
unittests/clangd/ClangdTests.cpp
Index: unittests/clangd/ClangdTests.cpp
===
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -7,7 +7,6 @@
//
//===--===//
-#include "ClangdLSPServer.h"
#include "ClangdServer.h"
#include "Logger.h"
#include "TestFS.h"
@@ -620,7 +619,7 @@
AddDocument(FileIndex);
Position Pos{LineDist(RandGen), ColumnDist(RandGen)};
- ASSERT_TRUE(!!Server.findDefinitions(FilePaths[FileIndex], Pos));
+ Server.findDefinitions(FilePaths[FileIndex], Pos);
};
std::vector> AsyncRequests = {
@@ -749,6 +748,58 @@
EXPECT_FALSE(PathResult.hasValue());
}
+TEST_F(ClangdVFSTest, CheckDefinitionIncludes) {
+ MockFSProvider FS;
+ ErrorCheckingDiagConsumer DiagConsumer;
+ MockCompilationDatabase CDB;
+
+ ClangdServer Server(CDB, DiagConsumer, FS, getDefaultAsyncThreadsCount(),
+ /*StorePreamblesInMemory=*/true,
+ EmptyLogger::getInstance());
+
+ auto FooCpp = getVirtualTestFilePath("foo.cpp");
+ const auto SourceContents = R"cpp(
+ #include "foo.h"
+ #include "invalid.h"
+ int b = a;
+ )cpp";
+ FS.Files[FooCpp] = SourceContents;
+ auto FooH = getVirtualTestFilePath("foo.h");
+ const auto HeaderContents = "int a;";
+
+ FS.Files[FooCpp] = SourceContents;
+ FS.Files[FooH] = HeaderContents;
+
+ Server.addDocument(FooH, HeaderContents);
+ Server.addDocument(FooCpp, SourceContents);
+
+ Position P = Position{1, 11};
+
+ std::vector Locations = Server.findDefinitions(FooCpp, P).get().Value;
+ EXPECT_TRUE(!Locations.empty());
+ std::string s("file:///");
+ std::string check = Locations[0].uri.uri;
+ check = check.erase(0, s.size());
+ check = check.substr(0, check.size() - 1);
+ ASSERT_EQ(check, FooH);
+ ASSERT_EQ(Locations[0].range.start.line, 0);
+ ASSERT_EQ(Locations[0].range.start.character, 0);
+ ASSERT_EQ(Locations[0].range.end.line, 0);
+ ASSERT_EQ(Locations[0].range.end.character, 0);
+
+ // Test ctrl-clicking on the #include part on the statement
+ Position P3 = Position{1, 3};
+
+ Locations = Server.findDefinitions(FooCpp, P3).get().Value;
+ EXPECT_TRUE(!Locations.empty());
+
+ // Test invalid include
+ Position P2 = Position{2, 11};
+
+ Locations = Server.findDefinitions(FooCpp, P2).get().Value;
+ EXPECT_TRUE(Locations.empty());
+}
+
TEST_F(ClangdThreadingTest, NoConcurrentDiagnostics) {
class NoConcurrentAccessDiagConsumer : public DiagnosticsConsumer {
public:
Index: clangd/Protocol.h
===
--- clangd/Protocol.h
+++ clangd/Protocol.h
@@ -108,6 +108,14 @@
bool fromJSON(const json::Expr &, Range &);
json::Expr toJSON(const Range &);
+class RangeHash {
+public:
+ std::size_t operator()(const Range &R) const {
+return ((R.start.line & 0x18) << 3) | ((R.start.character & 0x18) << 1) |
+ ((R.end.line & 0x18) >> 1) | ((R.end.character & 0x18) >> 3);
+ }
+};
+
struct Location {
/// The text document's URI.
URI uri;
Index: clangd/ClangdUnit.h
===
--- clangd/ClangdUnit.h
+++ clangd/ClangdUnit.h
@@ -22,6 +22,7 @@
#include
#include
#include
+#include
namespace llvm {
class raw_ostream;
@@ -59,6 +60,15 @@
std::vector Diags;
};
+class IncludeReferenceMap {
+ llvm::Optional findIncludeTargetAtLoc(Location Loc);
+
+public:
+ std::unordered_map IncludeLocationMap;
+ std::vector> DataVector;
+ std::vector RangeVector;
+};
+
/// Stores and provides access to parsed AST.
class ParsedAST {
public:
@@ -69,7 +79,8 @@
std::shared_ptr Preamble,
std::unique_ptr Buffer,
std::shared_ptr PCHs,
-IntrusiveRefCntPtr VFS, clangd::Logger &Logger);
+IntrusiveRefCntPtr VFS, clangd::Logger &Logger,
+IncludeReferenceMap IRM);
ParsedAST(ParsedAST &&Other);
ParsedAST &operator=(ParsedAST &&Other);
@@ -89,12 +100,14 @@
const std::vector &getDiagnostics() const;
+ IncludeReferenceMap takeIRM() { return IRM; };
+
private:
ParsedAST(std::shared_ptr Preamble,
std::unique_ptr Clang,
std::unique_ptr Action,
std::vector TopLevelDecls,
-std::vector Diags);
+std::vector Diags, IncludeReferenceMap IRM);
private:
void ensurePreambleDeclsDeserialized();
@@ -114,6 +127,8 @@
std::vector Diags;
std::vect
[PATCH] D38639: [clangd] #include statements support for Open definition
ilya-biryukov added inline comments.
Comment at: clangd/ClangdUnit.cpp:38
+class DelegatingPPCallbacks : public PPCallbacks {
+
Nebiroth wrote:
> ilya-biryukov wrote:
> > What's the purpose of this class?
> We need to be able to use a wrapper class to be able to make a unique_ptr to
> be sent to PrecompiledPreamble::Build in order to add the list of
> preprocessor Callbacks.
Could we implement an instance of `PPCallbacks` that contains
`CppFilePreambleCallbacks` and forwards to that specific method instead?
The reason is that we're not really delegating other methods in this calls(nor
should we, the implementation would be too compilcated).
Having a class that contains `CppFilePreambleCallbacks &Collector` and calling
`Collector.InclusionDirective` seems perfectly fine, though: its purpose is
clear and the implementation is easy.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D38639
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38639: [clangd] #include statements support for Open definition
Nebiroth updated this revision to Diff 125814.
Nebiroth added a comment.
Fixed re-added include
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D38639
Files:
clangd/ClangdServer.cpp
clangd/ClangdUnit.cpp
clangd/ClangdUnit.h
clangd/Protocol.h
unittests/clangd/ClangdTests.cpp
Index: unittests/clangd/ClangdTests.cpp
===
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -7,7 +7,6 @@
//
//===--===//
-#include "ClangdLSPServer.h"
#include "ClangdServer.h"
#include "Logger.h"
#include "TestFS.h"
@@ -620,7 +619,7 @@
AddDocument(FileIndex);
Position Pos{LineDist(RandGen), ColumnDist(RandGen)};
- ASSERT_TRUE(!!Server.findDefinitions(FilePaths[FileIndex], Pos));
+ Server.findDefinitions(FilePaths[FileIndex], Pos);
};
std::vector> AsyncRequests = {
@@ -749,6 +748,58 @@
EXPECT_FALSE(PathResult.hasValue());
}
+TEST_F(ClangdVFSTest, CheckDefinitionIncludes) {
+ MockFSProvider FS;
+ ErrorCheckingDiagConsumer DiagConsumer;
+ MockCompilationDatabase CDB;
+
+ ClangdServer Server(CDB, DiagConsumer, FS, getDefaultAsyncThreadsCount(),
+ /*StorePreamblesInMemory=*/true,
+ EmptyLogger::getInstance());
+
+ auto FooCpp = getVirtualTestFilePath("foo.cpp");
+ const auto SourceContents = R"cpp(
+ #include "foo.h"
+ #include "invalid.h"
+ int b = a;
+ )cpp";
+ FS.Files[FooCpp] = SourceContents;
+ auto FooH = getVirtualTestFilePath("foo.h");
+ const auto HeaderContents = "int a;";
+
+ FS.Files[FooCpp] = SourceContents;
+ FS.Files[FooH] = HeaderContents;
+
+ Server.addDocument(FooH, HeaderContents);
+ Server.addDocument(FooCpp, SourceContents);
+
+ Position P = Position{1, 11};
+
+ std::vector Locations = Server.findDefinitions(FooCpp, P).get().Value;
+ EXPECT_TRUE(!Locations.empty());
+ std::string s("file:///");
+ std::string check = Locations[0].uri.uri;
+ check = check.erase(0, s.size());
+ check = check.substr(0, check.size() - 1);
+ ASSERT_EQ(check, FooH);
+ ASSERT_EQ(Locations[0].range.start.line, 0);
+ ASSERT_EQ(Locations[0].range.start.character, 0);
+ ASSERT_EQ(Locations[0].range.end.line, 0);
+ ASSERT_EQ(Locations[0].range.end.character, 0);
+
+ // Test ctrl-clicking on the #include part on the statement
+ Position P3 = Position{1, 3};
+
+ Locations = Server.findDefinitions(FooCpp, P3).get().Value;
+ EXPECT_TRUE(!Locations.empty());
+
+ // Test invalid include
+ Position P2 = Position{2, 11};
+
+ Locations = Server.findDefinitions(FooCpp, P2).get().Value;
+ EXPECT_TRUE(Locations.empty());
+}
+
TEST_F(ClangdThreadingTest, NoConcurrentDiagnostics) {
class NoConcurrentAccessDiagConsumer : public DiagnosticsConsumer {
public:
Index: clangd/Protocol.h
===
--- clangd/Protocol.h
+++ clangd/Protocol.h
@@ -108,6 +108,14 @@
bool fromJSON(const json::Expr &, Range &);
json::Expr toJSON(const Range &);
+class RangeHash {
+public:
+ std::size_t operator()(const Range &R) const {
+return ((R.start.line & 0x18) << 3) | ((R.start.character & 0x18) << 1) |
+ ((R.end.line & 0x18) >> 1) | ((R.end.character & 0x18) >> 3);
+ }
+};
+
struct Location {
/// The text document's URI.
URI uri;
Index: clangd/ClangdUnit.h
===
--- clangd/ClangdUnit.h
+++ clangd/ClangdUnit.h
@@ -22,6 +22,7 @@
#include
#include
#include
+#include
namespace llvm {
class raw_ostream;
@@ -59,6 +60,15 @@
std::vector Diags;
};
+class IncludeReferenceMap {
+ llvm::Optional findIncludeTargetAtLoc(Location Loc);
+
+public:
+ std::unordered_map IncludeLocationMap;
+ std::vector> DataVector;
+ std::vector RangeVector;
+};
+
/// Stores and provides access to parsed AST.
class ParsedAST {
public:
@@ -69,7 +79,8 @@
std::shared_ptr Preamble,
std::unique_ptr Buffer,
std::shared_ptr PCHs,
-IntrusiveRefCntPtr VFS, clangd::Logger &Logger);
+IntrusiveRefCntPtr VFS, clangd::Logger &Logger,
+IncludeReferenceMap IRM);
ParsedAST(ParsedAST &&Other);
ParsedAST &operator=(ParsedAST &&Other);
@@ -89,12 +100,14 @@
const std::vector &getDiagnostics() const;
+ IncludeReferenceMap takeIRM() { return IRM; };
+
private:
ParsedAST(std::shared_ptr Preamble,
std::unique_ptr Clang,
std::unique_ptr Action,
std::vector TopLevelDecls,
-std::vector Diags);
+std::vector Diags, IncludeReferenceMap IRM);
private:
void ensurePreambleDeclsDeserialized();
@@ -114,6 +127,8 @@
std::vector Diags;
std::vector TopLevelDecls;
bool PreambleDeclsDeserialized;
+ std::vector PendingTopLevelDecls;
+ IncludeReferenceMap IRM;
};
// Provides thread-safe access to ParsedAST.
@@ -256,14 +271,14 @@
clangd::Logger &Lo
[PATCH] D38639: [clangd] #include statements support for Open definition
Nebiroth updated this revision to Diff 125813.
Nebiroth added a comment.
Herald added a subscriber: klimek.
Using PPCallbacks interface to find non-preamble includes
Created inner class to store vectors in to find locations
Refactored methods to remove some unnecessary parameters
Refactored Unit tests
Merge with most recent master branch + clang
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D38639
Files:
clangd/ClangdServer.cpp
clangd/ClangdUnit.cpp
clangd/ClangdUnit.h
clangd/GlobalCompilationDatabase.cpp
clangd/Protocol.h
unittests/clangd/ClangdTests.cpp
Index: unittests/clangd/ClangdTests.cpp
===
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -7,7 +7,6 @@
//
//===--===//
-#include "ClangdLSPServer.h"
#include "ClangdServer.h"
#include "Logger.h"
#include "TestFS.h"
@@ -620,7 +619,7 @@
AddDocument(FileIndex);
Position Pos{LineDist(RandGen), ColumnDist(RandGen)};
- ASSERT_TRUE(!!Server.findDefinitions(FilePaths[FileIndex], Pos));
+ Server.findDefinitions(FilePaths[FileIndex], Pos);
};
std::vector> AsyncRequests = {
@@ -749,6 +748,58 @@
EXPECT_FALSE(PathResult.hasValue());
}
+TEST_F(ClangdVFSTest, CheckDefinitionIncludes) {
+ MockFSProvider FS;
+ ErrorCheckingDiagConsumer DiagConsumer;
+ MockCompilationDatabase CDB;
+
+ ClangdServer Server(CDB, DiagConsumer, FS, getDefaultAsyncThreadsCount(),
+ /*StorePreamblesInMemory=*/true,
+ EmptyLogger::getInstance());
+
+ auto FooCpp = getVirtualTestFilePath("foo.cpp");
+ const auto SourceContents = R"cpp(
+ #include "foo.h"
+ #include "invalid.h"
+ int b = a;
+ )cpp";
+ FS.Files[FooCpp] = SourceContents;
+ auto FooH = getVirtualTestFilePath("foo.h");
+ const auto HeaderContents = "int a;";
+
+ FS.Files[FooCpp] = SourceContents;
+ FS.Files[FooH] = HeaderContents;
+
+ Server.addDocument(FooH, HeaderContents);
+ Server.addDocument(FooCpp, SourceContents);
+
+ Position P = Position{1, 11};
+
+ std::vector Locations = Server.findDefinitions(FooCpp, P).get().Value;
+ EXPECT_TRUE(!Locations.empty());
+ std::string s("file:///");
+ std::string check = Locations[0].uri.uri;
+ check = check.erase(0, s.size());
+ check = check.substr(0, check.size() - 1);
+ ASSERT_EQ(check, FooH);
+ ASSERT_EQ(Locations[0].range.start.line, 0);
+ ASSERT_EQ(Locations[0].range.start.character, 0);
+ ASSERT_EQ(Locations[0].range.end.line, 0);
+ ASSERT_EQ(Locations[0].range.end.character, 0);
+
+ // Test ctrl-clicking on the #include part on the statement
+ Position P3 = Position{1, 3};
+
+ Locations = Server.findDefinitions(FooCpp, P3).get().Value;
+ EXPECT_TRUE(!Locations.empty());
+
+ // Test invalid include
+ Position P2 = Position{2, 11};
+
+ Locations = Server.findDefinitions(FooCpp, P2).get().Value;
+ EXPECT_TRUE(Locations.empty());
+}
+
TEST_F(ClangdThreadingTest, NoConcurrentDiagnostics) {
class NoConcurrentAccessDiagConsumer : public DiagnosticsConsumer {
public:
Index: clangd/Protocol.h
===
--- clangd/Protocol.h
+++ clangd/Protocol.h
@@ -108,6 +108,14 @@
bool fromJSON(const json::Expr &, Range &);
json::Expr toJSON(const Range &);
+class RangeHash {
+public:
+ std::size_t operator()(const Range &R) const {
+return ((R.start.line & 0x18) << 3) | ((R.start.character & 0x18) << 1) |
+ ((R.end.line & 0x18) >> 1) | ((R.end.character & 0x18) >> 3);
+ }
+};
+
struct Location {
/// The text document's URI.
URI uri;
Index: clangd/GlobalCompilationDatabase.cpp
===
--- clangd/GlobalCompilationDatabase.cpp
+++ clangd/GlobalCompilationDatabase.cpp
@@ -9,6 +9,7 @@
#include "GlobalCompilationDatabase.h"
#include "Logger.h"
+#include "ProtocolHandlers.h"
#include "clang/Tooling/CompilationDatabase.h"
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/Path.h"
Index: clangd/ClangdUnit.h
===
--- clangd/ClangdUnit.h
+++ clangd/ClangdUnit.h
@@ -22,6 +22,7 @@
#include
#include
#include
+#include
namespace llvm {
class raw_ostream;
@@ -59,6 +60,15 @@
std::vector Diags;
};
+class IncludeReferenceMap {
+ llvm::Optional findIncludeTargetAtLoc(Location Loc);
+
+public:
+ std::unordered_map IncludeLocationMap;
+ std::vector> DataVector;
+ std::vector RangeVector;
+};
+
/// Stores and provides access to parsed AST.
class ParsedAST {
public:
@@ -69,7 +79,8 @@
std::shared_ptr Preamble,
std::unique_ptr Buffer,
std::shared_ptr PCHs,
-IntrusiveRefCntPtr VFS, clangd::Logger &Logger);
+IntrusiveRefCntPtr VFS, clangd::Logger &Logger,
+IncludeReferenceMap IRM);
ParsedAST(ParsedAST &&Other);
ParsedAST &operator=(Parse
[PATCH] D38639: [clangd] #include statements support for Open definition
Nebiroth marked an inline comment as done.
Nebiroth added inline comments.
Comment at: clangd/ClangdUnit.cpp:38
+class DelegatingPPCallbacks : public PPCallbacks {
+
ilya-biryukov wrote:
> What's the purpose of this class?
We need to be able to use a wrapper class to be able to make a unique_ptr to be
sent to PrecompiledPreamble::Build in order to add the list of preprocessor
Callbacks.
https://reviews.llvm.org/D38639
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38639: [clangd] #include statements support for Open definition
malaperle added a comment. In https://reviews.llvm.org/D38639#920487, @ilya-biryukov wrote: > I think we should never iterate through `SourceManager`, as it's much easier > to get wrong than using the callbacks. I agree that all that fiddling with > callbacks is unfortunate, but it's well worth the fact that it'd be much > easier to tell that the implementation is correct by simply looking at the > implementation and not knowing how `SourceManager` works. `PPCallbacks` is a > much more direct API that was designed to handle our purposes. > > Note that we don't need to use `SourceManager` to find non-preamble includes, > we should implement proper `PPCallbacks` and use them when building the AST. Sounds good! https://reviews.llvm.org/D38639 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38639: [clangd] #include statements support for Open definition
ilya-biryukov added inline comments.
Comment at: clangd/ClangdServer.cpp:368
std::vector Result;
- Resources->getAST().get()->runUnderLock([Pos, &Result, this](ParsedAST *AST)
{
-if (!AST)
- return;
-Result = clangd::findDefinitions(*AST, Pos, Logger);
- });
+ std::unordered_map IncludeLocationMap;
+ std::vector> DataVector;
This map and two vectors come up quite often in your commit.
Is this a storage for saved includes? Could we define a class that stores this
data?
Something like
```
class IncludeReferenceMap {
llvm::Optional findIncludeTargetAtLoc(Location Loc);
private:
// maps, vectors, whatever we need go here
};
```
We'd build it using the callbacks, then store it in `PreambleData` (for PCH
include) and `ParsedAST` (for non-PCH includes). And later query both maps to
get the result.
Comment at: clangd/ClangdUnit.cpp:33
#include
+#include
We probably don't need it this include
Comment at: clangd/ClangdUnit.cpp:38
+class DelegatingPPCallbacks : public PPCallbacks {
+
What's the purpose of this class?
Comment at: clangd/ClangdUnit.cpp:131
+
+ unsigned NumSlocs = SM.local_sloc_entry_size();
+ SmallVector InclusionStack;
Please also use `PPCallbacks` interface instead of the `SourceManager` (should
be possible to hook it up somewhere in `ParsedAST::Build`).
https://reviews.llvm.org/D38639
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38639: [clangd] #include statements support for Open definition
ilya-biryukov added a comment. In https://reviews.llvm.org/D38639#920427, @malaperle wrote: > I'm not sure it's better to use the InclusionDirective callback for this. We > need to get the includes in two places: in the preamble and non-preamble. In > the preamble we use the callback, have to store some temporary stuff because > we don't have a SourceManager in InclusionDirective, then in finish we use > the SourceManager to convert everything. In the non-preamble, we cannot use > the callback so we use the SourceManager to go through the includes. > Therefore, we have to maintain two different ways of getting the inclusion > map. Without using the InclusionDirective callback, we use the SourceManager > in both cases and can use the same code to iterate through inclusions, just > on two different SourceManagers at different moments. We also don't need to > make another patch to modify the PreambleCallbacks. I also double this is > much slower but I think this simplifies the code nicely. > What do you think? I think we should never iterate through `SourceManager`, as it's much easier to get wrong than using the callbacks. I agree that all that fiddling with callbacks is unfortunate, but it's well worth the fact that it'd be much easier to tell that the implementation is correct by simply looking at the implementation and not knowing how `SourceManager` works. `PPCallbacks` is a much more direct API that was designed to handle our purposes. Note that we don't need to use `SourceManager` to find non-preamble includes, we should implement proper `PPCallbacks` and use them when building the AST. https://reviews.llvm.org/D38639 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38639: [clangd] #include statements support for Open definition
malaperle added a comment. I'm not sure it's better to use the InclusionDirective callback for this. We need to get the includes in two places: in the preamble and non-preamble. In the preamble we use the callback, have to store some temporary stuff because we don't have a SourceManager in InclusionDirective, then in finish we use the SourceManager to convert everything. In the non-preamble, we cannot use the callback so we use the SourceManager to go through the includes. Therefore, we have to maintain two different ways of getting the inclusion map. Without using the InclusionDirective callback, we use the SourceManager in both cases and can use the same code to iterate through inclusions, just on two different SourceManagers at different moments. We also don't need to make another patch to modify the PreambleCallbacks. I also double this is much slower but I think this simplifies the code nicely. What do you think? https://reviews.llvm.org/D38639 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38639: [clangd] #include statements support for Open definition
Nebiroth updated this revision to Diff 120485.
Nebiroth added a comment.
- Fixed adding incorrect test file.
https://reviews.llvm.org/D38639
Files:
clangd/ClangdServer.cpp
clangd/ClangdUnit.cpp
clangd/ClangdUnit.h
clangd/GlobalCompilationDatabase.cpp
clangd/Protocol.h
unittests/clangd/ClangdTests.cpp
Index: unittests/clangd/ClangdTests.cpp
===
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -7,7 +7,6 @@
//
//===--===//
-#include "ClangdLSPServer.h"
#include "ClangdServer.h"
#include "Logger.h"
#include "clang/Basic/VirtualFileSystem.h"
@@ -1056,7 +1055,7 @@
AddDocument(FileIndex);
Position Pos{LineDist(RandGen), ColumnDist(RandGen)};
- ASSERT_TRUE(!!Server.findDefinitions(FilePaths[FileIndex], Pos));
+ Server.findDefinitions(FilePaths[FileIndex], Pos);
};
std::vector> AsyncRequests = {
@@ -1185,6 +1184,57 @@
EXPECT_FALSE(PathResult.hasValue());
}
+TEST_F(ClangdVFSTest, CheckDefinitionIncludes) {
+ MockFSProvider FS;
+ ErrorCheckingDiagConsumer DiagConsumer;
+ MockCompilationDatabase CDB(/*AddFreestandingFlag=*/true);
+
+ ClangdServer Server(CDB, DiagConsumer, FS, 0, clangd::CodeCompleteOptions(),
+ EmptyLogger::getInstance());
+
+ auto FooCpp = getVirtualTestFilePath("foo.cpp");
+ const auto SourceContents = R"cpp(
+ #include "foo.h"
+ #include "invalid.h"
+ int b = a;
+ )cpp";
+ FS.Files[FooCpp] = SourceContents;
+ auto FooH = getVirtualTestFilePath("foo.h");
+ const auto HeaderContents = "int a;";
+
+ FS.Files[FooCpp] = SourceContents;
+ FS.Files[FooH] = HeaderContents;
+
+ Server.addDocument(FooH, HeaderContents);
+ Server.addDocument(FooCpp, SourceContents);
+
+ Position P = Position{1, 11};
+
+ std::vector Locations = Server.findDefinitions(FooCpp, P).get().Value;
+ EXPECT_TRUE(!Locations.empty());
+ std::string s("file:///");
+ std::string check = URI::unparse(Locations[0].uri);
+ check = check.erase(0, s.size());
+ check = check.substr(0, check.size() - 1);
+ ASSERT_EQ(check, FooH);
+ ASSERT_EQ(Locations[0].range.start.line, 0);
+ ASSERT_EQ(Locations[0].range.start.character, 0);
+ ASSERT_EQ(Locations[0].range.end.line, 0);
+ ASSERT_EQ(Locations[0].range.end.character, 0);
+
+ // Test ctrl-clicking on the #include part on the statement
+ Position P3 = Position{1, 3};
+
+ Locations = Server.findDefinitions(FooCpp, P3).get().Value;
+ EXPECT_TRUE(!Locations.empty());
+
+ // Test invalid include
+ Position P2 = Position{2, 11};
+
+ Locations = Server.findDefinitions(FooCpp, P2).get().Value;
+ EXPECT_TRUE(Locations.empty());
+}
+
TEST_F(ClangdThreadingTest, NoConcurrentDiagnostics) {
class NoConcurrentAccessDiagConsumer : public DiagnosticsConsumer {
public:
Index: clangd/Protocol.h
===
--- clangd/Protocol.h
+++ clangd/Protocol.h
@@ -96,12 +96,21 @@
friend bool operator<(const Range &LHS, const Range &RHS) {
return std::tie(LHS.start, LHS.end) < std::tie(RHS.start, RHS.end);
}
-
static llvm::Optional parse(llvm::yaml::MappingNode *Params,
clangd::Logger &Logger);
static std::string unparse(const Range &P);
+
+};
+
+class RangeHash {
+public:
+ std::size_t operator()(const Range &R) const
+ {
+return ((R.start.line & 0x18) << 3) | ((R.start.character & 0x18) << 1) | ((R.end.line & 0x18) >> 1) | ((R.end.character & 0x18) >> 3);
+ }
};
+
struct Location {
/// The text document's URI.
URI uri;
Index: clangd/GlobalCompilationDatabase.cpp
===
--- clangd/GlobalCompilationDatabase.cpp
+++ clangd/GlobalCompilationDatabase.cpp
@@ -9,6 +9,7 @@
#include "GlobalCompilationDatabase.h"
#include "Logger.h"
+#include "ProtocolHandlers.h"
#include "clang/Tooling/CompilationDatabase.h"
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/Path.h"
@@ -30,6 +31,8 @@
Command.CommandLine.insert(It, ExtraFlags.begin(), ExtraFlags.end());
}
+
+
tooling::CompileCommand getDefaultCompileCommand(PathRef File) {
std::vector CommandLine{"clang", "-fsyntax-only", File.str()};
return tooling::CompileCommand(llvm::sys::path::parent_path(File),
Index: clangd/ClangdUnit.h
===
--- clangd/ClangdUnit.h
+++ clangd/ClangdUnit.h
@@ -22,6 +22,7 @@
#include
#include
#include
+#include
namespace llvm {
class raw_ostream;
@@ -128,11 +129,17 @@
struct PreambleData {
PreambleData(PrecompiledPreamble Preamble,
std::vector TopLevelDeclIDs,
- std::vector Diags);
+ std::vector Diags,
+ std::unordered_map IncludeMap,
+ std::vector> DataVector,
+ std::vector RangeVector);
Preco
[PATCH] D38639: [clangd] #include statements support for Open definition
Nebiroth updated this revision to Diff 120482.
Nebiroth added a comment.
- Now overriding InclusionDirective as a callback to get proper includes
information.
- Fixed tests.
https://reviews.llvm.org/D38639
Files:
clangd/ClangdServer.cpp
clangd/ClangdUnit.cpp
clangd/ClangdUnit.h
clangd/GlobalCompilationDatabase.cpp
clangd/Protocol.h
test/clangd/documenthighlight.test
unittests/clangd/ClangdTests.cpp
Index: unittests/clangd/ClangdTests.cpp
===
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -7,7 +7,6 @@
//
//===--===//
-#include "ClangdLSPServer.h"
#include "ClangdServer.h"
#include "Logger.h"
#include "clang/Basic/VirtualFileSystem.h"
@@ -1056,7 +1055,7 @@
AddDocument(FileIndex);
Position Pos{LineDist(RandGen), ColumnDist(RandGen)};
- ASSERT_TRUE(!!Server.findDefinitions(FilePaths[FileIndex], Pos));
+ Server.findDefinitions(FilePaths[FileIndex], Pos);
};
std::vector> AsyncRequests = {
@@ -1185,6 +1184,57 @@
EXPECT_FALSE(PathResult.hasValue());
}
+TEST_F(ClangdVFSTest, CheckDefinitionIncludes) {
+ MockFSProvider FS;
+ ErrorCheckingDiagConsumer DiagConsumer;
+ MockCompilationDatabase CDB(/*AddFreestandingFlag=*/true);
+
+ ClangdServer Server(CDB, DiagConsumer, FS, 0, clangd::CodeCompleteOptions(),
+ EmptyLogger::getInstance());
+
+ auto FooCpp = getVirtualTestFilePath("foo.cpp");
+ const auto SourceContents = R"cpp(
+ #include "foo.h"
+ #include "invalid.h"
+ int b = a;
+ )cpp";
+ FS.Files[FooCpp] = SourceContents;
+ auto FooH = getVirtualTestFilePath("foo.h");
+ const auto HeaderContents = "int a;";
+
+ FS.Files[FooCpp] = SourceContents;
+ FS.Files[FooH] = HeaderContents;
+
+ Server.addDocument(FooH, HeaderContents);
+ Server.addDocument(FooCpp, SourceContents);
+
+ Position P = Position{1, 11};
+
+ std::vector Locations = Server.findDefinitions(FooCpp, P).get().Value;
+ EXPECT_TRUE(!Locations.empty());
+ std::string s("file:///");
+ std::string check = URI::unparse(Locations[0].uri);
+ check = check.erase(0, s.size());
+ check = check.substr(0, check.size() - 1);
+ ASSERT_EQ(check, FooH);
+ ASSERT_EQ(Locations[0].range.start.line, 0);
+ ASSERT_EQ(Locations[0].range.start.character, 0);
+ ASSERT_EQ(Locations[0].range.end.line, 0);
+ ASSERT_EQ(Locations[0].range.end.character, 0);
+
+ // Test ctrl-clicking on the #include part on the statement
+ Position P3 = Position{1, 3};
+
+ Locations = Server.findDefinitions(FooCpp, P3).get().Value;
+ EXPECT_TRUE(!Locations.empty());
+
+ // Test invalid include
+ Position P2 = Position{2, 11};
+
+ Locations = Server.findDefinitions(FooCpp, P2).get().Value;
+ EXPECT_TRUE(Locations.empty());
+}
+
TEST_F(ClangdThreadingTest, NoConcurrentDiagnostics) {
class NoConcurrentAccessDiagConsumer : public DiagnosticsConsumer {
public:
Index: test/clangd/documenthighlight.test
===
--- /dev/null
+++ test/clangd/documenthighlight.test
@@ -0,0 +1,42 @@
+# RUN: clangd -run-synchronously < %s | FileCheck %s
+# It is absolutely vital that this file has CRLF line endings.
+#
+Content-Length: 125
+
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+# CHECK: Content-Length: 580
+# CHECK: {"jsonrpc":"2.0","id":0,"result":{"capabilities":{
+# CHECK: "textDocumentSync": 1,
+# CHECK: "documentFormattingProvider": true,
+# CHECK: "documentRangeFormattingProvider": true,
+# CHECK: "documentOnTypeFormattingProvider": {"firstTriggerCharacter":"}","moreTriggerCharacter":[]},
+# CHECK: "codeActionProvider": true,
+# CHECK: "completionProvider": {"resolveProvider": false, "triggerCharacters": [".",">",":"]},
+# CHECK: "signatureHelpProvider": {"triggerCharacters": ["(",","]},
+# CHECK: "definitionProvider": true,
+# CHECK: "documentHighlightProvider": true
+# CHECK: }}}
+#
+
+Content-Length: 455
+
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///main.cpp","languageId":"cpp","version":1,"text":"#define MACRO 1\nnamespace ns1 {\nstruct MyClass {\nint xasd;\nvoid anotherOperation() {\n}\nstatic int foo(MyClass*) {\nreturn 0;\n}\n\n};\nstruct Foo {\nint xasd;\n};\n}\nint main() {\nint bonjour;\nbonjour = 2;\nns1::Foo bar = { xasd : 1};\nbar.xasd = 3;\nns1::MyClass* Params;\nParams->anotherOperation();}\n"}}}
+
+Content-Length: 156
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":17,"character":2}}}
+# Go to local variable
+# CHECK: {"jsonrpc":"2.0","id":1,"result":[{"range": {"start": {"line": 16, "character": 4}, "end": {"line": 16, "character": 12}}, "kind": 1},{"range": {"start": {"line": 17, "character": 0}, "end": {"line":
[PATCH] D38639: [clangd] #include statements support for Open definition
ilya-biryukov added inline comments.
Comment at: clangd/ClangdUnit.cpp:103
+ void AfterExecute(CompilerInstance &CI) override {
+const SourceManager &SM = CI.getSourceManager();
Nebiroth wrote:
> ilya-biryukov wrote:
> > There's a much better public API to get all includes that were encountered
> > by the `Preprocessor`: we need to override `PPCallbacks
> > ::InclusionDirective`.
> >
> >
> > `PrecompiledPreamble` does not currently expose this callbacks, but could
> > you add to `PreambleCallbacks` in a separate commit?
> >
> If I were to use InclusionDirective , how would that callback be called
> automatically? As far as I know, it wouldn't be called automatically for
> every file that gets indexed the same way AfterExecute would be.
It will be called for each `#include` directive that Preprocessor encountered
while building the preamble.
A separate instance of `CppFilePreambleCallbacks` is created every time we
build a preamble for file.
Does that answer your question?
https://reviews.llvm.org/D38639
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38639: [clangd] #include statements support for Open definition
Nebiroth marked 21 inline comments as done.
Nebiroth added inline comments.
Comment at: clangd/ClangdUnit.cpp:103
+ void AfterExecute(CompilerInstance &CI) override {
+const SourceManager &SM = CI.getSourceManager();
ilya-biryukov wrote:
> There's a much better public API to get all includes that were encountered by
> the `Preprocessor`: we need to override `PPCallbacks ::InclusionDirective`.
>
>
> `PrecompiledPreamble` does not currently expose this callbacks, but could you
> add to `PreambleCallbacks` in a separate commit?
>
If I were to use InclusionDirective , how would that callback be called
automatically? As far as I know, it wouldn't be called automatically for every
file that gets indexed the same way AfterExecute would be.
https://reviews.llvm.org/D38639
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38639: [clangd] #include statements support for Open definition
ilya-biryukov requested changes to this revision.
ilya-biryukov added a comment.
Looking forward to getting this change! I miss this as well.
Please take a look at my comments, though. I think we might want to use a
different API to implement this.
Comment at: clangd/ClangdServer.cpp:292
+ std::shared_ptr StalePreamble =
+ Resources->getPossiblyStalePreamble();
+ if (StalePreamble)
We can't use `getPossiblyStalePreamble()`, we want latest state of the
`Preamble`. Use `getPreamble` instead.
Comment at: clangd/ClangdServer.cpp:294
+ if (StalePreamble)
+IncludeMap = StalePreamble->IncludeMap;
+ Resources->getAST().get()->runUnderLock(
We don't need to copy the whole map here, better add a method to `PreambleData`
that does actual lookups for the source range.
Comment at: clangd/ClangdUnit.cpp:103
+ void AfterExecute(CompilerInstance &CI) override {
+const SourceManager &SM = CI.getSourceManager();
There's a much better public API to get all includes that were encountered by
the `Preprocessor`: we need to override `PPCallbacks ::InclusionDirective`.
`PrecompiledPreamble` does not currently expose this callbacks, but could you
add to `PreambleCallbacks` in a separate commit?
Comment at: clangd/ClangdUnit.cpp:151
std::vector TopLevelDeclIDs;
+ std::map IncludeMap;
};
Please use our descriptive `Path` typedef.
Comment at: clangd/ClangdUnit.cpp:834
+
+for (auto it = IncludeMap.begin(); it != IncludeMap.end(); ++it) {
+ SourceLocation L = it->first;
Can we mix in the results from includes outside `DeclarationLocationsFinder`?
Comment at: clangd/ClangdUnit.h:136
std::vector Diags;
+ std::map IncludeMap;
};
`std::unordered_map` is a better fit here, why not use it?
Comment at: clangd/ClangdUnit.h:136
std::vector Diags;
+ std::map IncludeMap;
};
ilya-biryukov wrote:
> `std::unordered_map` is a better fit here, why not use it?
Using `SourceLocation` after `SourceManager` owning them dies is somewhat hacky.
Please store clangd's `Range`s instead. You can get the ranges via
`PPCallbacks` method `InclusionDirective`, it has a parameter `CharSourceRange
FilenameRange`, exactly what we need here.
Comment at: unittests/clangd/ClangdTests.cpp:991
+ #include "foo.h"
+ #include "invalid.h"
+ int b = a;
Some includes may come outside of preamble, I would expect current
implementation to fail for this case:
```
#include // <-- works here
// preamble ends here
int main() {
#include "my_include.h" // <-- does not work here
}
```
https://reviews.llvm.org/D38639
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38639: [clangd] #include statements support for Open definition
malaperle requested changes to this revision.
malaperle added inline comments.
This revision now requires changes to proceed.
Comment at: clangd/ClangdUnit.cpp:81
+ std::map takeIncludeMap() {
+return std::move(IncludeMap);
takeIncludeLocationMap?
Comment at: clangd/ClangdUnit.cpp:105
+const SourceManager &SM = CI.getSourceManager();
+unsigned n = SM.local_sloc_entry_size();
+SmallVector InclusionStack;
n -> NumSlocs?
Comment at: clangd/ClangdUnit.cpp:107
+SmallVector InclusionStack;
+std::map::iterator it = IncludeMap.begin();
+
do you need that iterator?
Comment at: clangd/ClangdUnit.cpp:109
+
+for (unsigned i = 0; i < n; ++i) {
+ bool Invalid = false;
i -> I
Comment at: clangd/ClangdUnit.cpp:137
+ FI.getContentCache()->OrigEntry->tryGetRealPathName();
+ if (FilePath.empty()) {
+// FIXME: Does tryGetRealPathName return empty if and only if the path
I think you can just skip it if empty (continue)
Comment at: clangd/ClangdUnit.cpp:143
+ }
+ IncludeMap.insert(std::pair(
+ InclusionStack.front(), FilePath.str()));
I think you can do instead
IncludeMap.insert({InclusionStack.front(), FilePath.str()});
Comment at: clangd/ClangdUnit.cpp:151
std::vector TopLevelDeclIDs;
+ std::map IncludeMap;
};
IncludeMap -> IncludeLocationMap ?
Comment at: clangd/ClangdUnit.cpp:800
- void addDeclarationLocation(const SourceRange &ValSourceRange) {
+ bool isSameLine(unsigned line) const {
+const SourceManager &SourceMgr = AST.getSourceManager();
line -> Line
Comment at: clangd/ClangdUnit.cpp:806
+ void addDeclarationLocation(const SourceRange &ValSourceRange,
+ bool test = false) {
const SourceManager &SourceMgr = AST.getSourceManager();
remove bool test?
Comment at: clangd/ClangdUnit.cpp:824
+
+ void addLocation(URI uri, Range R) {
+
uri -> Uri
Comment at: clangd/ClangdUnit.cpp:834
+
+for (auto it = IncludeMap.begin(); it != IncludeMap.end(); ++it) {
+ SourceLocation L = it->first;
it -> It
Comment at: clangd/ClangdUnit.cpp:837
+ std::string &Path = it->second;
+ Range r = Range();
+ unsigned line = AST.getSourceManager().getSpellingLineNumber(L);
r -> R
Comment at: clangd/ClangdUnit.cpp:839
+ unsigned line = AST.getSourceManager().getSpellingLineNumber(L);
+ if (isSameLine(line))
+addLocation(URI::fromFile(Path), Range());
line -> Line
Comment at: clangd/ClangdUnit.h:136
std::vector Diags;
+ std::map IncludeMap;
};
IncludeLocationMap?
Comment at: clangd/ClangdUnit.h:267
+ clangd::Logger &Logger,
+ std::map);
do you want to add a name to the parameter here?
Comment at: unittests/clangd/ClangdTests.cpp:902
-TEST_F(ClangdVFSTest, CheckSourceHeaderSwitch) {
+TEST_F(ClangdVFSTest, CheckDefinitionIncludes) {
MockFSProvider FS;
the test "CheckSourceHeaderSwitch" was removed
https://reviews.llvm.org/D38639
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38639: [clangd] #include statements support for Open definition
Nebiroth updated this revision to Diff 118046.
Nebiroth added a comment.
Fixed accidental removal of CheckSourceHeaderSwitch test
https://reviews.llvm.org/D38639
Files:
clangd/ClangdServer.cpp
clangd/ClangdUnit.cpp
clangd/ClangdUnit.h
unittests/clangd/ClangdTests.cpp
Index: unittests/clangd/ClangdTests.cpp
===
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -7,7 +7,6 @@
//
//===--===//
-#include "ClangdLSPServer.h"
#include "ClangdServer.h"
#include "Logger.h"
#include "clang/Basic/VirtualFileSystem.h"
@@ -978,6 +977,57 @@
EXPECT_FALSE(PathResult.hasValue());
}
+TEST_F(ClangdVFSTest, CheckDefinitionIncludes) {
+ MockFSProvider FS;
+ ErrorCheckingDiagConsumer DiagConsumer;
+ MockCompilationDatabase CDB(/*AddFreestandingFlag=*/true);
+
+ ClangdServer Server(CDB, DiagConsumer, FS, 0,
+ /*SnippetCompletions=*/false, EmptyLogger::getInstance());
+
+ auto FooCpp = getVirtualTestFilePath("foo.cpp");
+ const auto SourceContents = R"cpp(
+ #include "foo.h"
+ #include "invalid.h"
+ int b = a;
+ )cpp";
+ FS.Files[FooCpp] = SourceContents;
+ auto FooH = getVirtualTestFilePath("foo.h");
+ const auto HeaderContents = "int a;";
+
+ FS.Files[FooCpp] = SourceContents;
+ FS.Files[FooH] = HeaderContents;
+
+ Server.addDocument(FooH, HeaderContents);
+ Server.addDocument(FooCpp, SourceContents);
+
+ Position P = Position{1, 11};
+
+ std::vector Locations = (Server.findDefinitions(FooCpp, P)).Value;
+ EXPECT_TRUE(!Locations.empty());
+ std::string s("file:///");
+ std::string check = URI::unparse(Locations[0].uri);
+ check = check.erase(0, s.size());
+ check = check.substr(0, check.size() - 1);
+ ASSERT_EQ(check, FooH);
+ ASSERT_EQ(Locations[0].range.start.line, 0);
+ ASSERT_EQ(Locations[0].range.start.character, 0);
+ ASSERT_EQ(Locations[0].range.end.line, 0);
+ ASSERT_EQ(Locations[0].range.end.character, 0);
+
+ // Test ctrl-clicking on the #include part on the statement
+ Position P3 = Position{1, 3};
+
+ Locations = (Server.findDefinitions(FooCpp, P3)).Value;
+ EXPECT_TRUE(!Locations.empty());
+
+ // Test invalid include
+ Position P2 = Position{2, 11};
+
+ Locations = (Server.findDefinitions(FooCpp, P2)).Value;
+ EXPECT_TRUE(Locations.empty());
+}
+
TEST_F(ClangdThreadingTest, NoConcurrentDiagnostics) {
class NoConcurrentAccessDiagConsumer : public DiagnosticsConsumer {
public:
Index: clangd/ClangdUnit.h
===
--- clangd/ClangdUnit.h
+++ clangd/ClangdUnit.h
@@ -127,11 +127,13 @@
struct PreambleData {
PreambleData(PrecompiledPreamble Preamble,
std::vector TopLevelDeclIDs,
- std::vector Diags);
+ std::vector Diags,
+ std::map IncludeMap);
PrecompiledPreamble Preamble;
std::vector TopLevelDeclIDs;
std::vector Diags;
+ std::map IncludeMap;
};
/// Manages resources, required by clangd. Allows to rebuild file with new
@@ -261,7 +263,8 @@
/// Get definition of symbol at a specified \p Pos.
std::vector findDefinitions(ParsedAST &AST, Position Pos,
- clangd::Logger &Logger);
+ clangd::Logger &Logger,
+ std::map);
/// For testing/debugging purposes. Note that this method deserializes all
/// unserialized Decls, so use with care.
Index: clangd/ClangdUnit.cpp
===
--- clangd/ClangdUnit.cpp
+++ clangd/ClangdUnit.cpp
@@ -78,6 +78,10 @@
return std::move(TopLevelDeclIDs);
}
+ std::map takeIncludeMap() {
+return std::move(IncludeMap);
+ }
+
void AfterPCHEmitted(ASTWriter &Writer) override {
TopLevelDeclIDs.reserve(TopLevelDecls.size());
for (Decl *D : TopLevelDecls) {
@@ -96,9 +100,55 @@
}
}
+ void AfterExecute(CompilerInstance &CI) override {
+const SourceManager &SM = CI.getSourceManager();
+unsigned n = SM.local_sloc_entry_size();
+SmallVector InclusionStack;
+std::map::iterator it = IncludeMap.begin();
+
+for (unsigned i = 0; i < n; ++i) {
+ bool Invalid = false;
+ const SrcMgr::SLocEntry &SL = SM.getLocalSLocEntry(i, &Invalid);
+ if (!SL.isFile() || Invalid)
+continue;
+ const SrcMgr::FileInfo &FI = SL.getFile();
+ SourceLocation L = FI.getIncludeLoc();
+ InclusionStack.clear();
+
+ SourceLocation LocationToInsert;
+
+ while (L.isValid()) {
+PresumedLoc PLoc = SM.getPresumedLoc(L);
+InclusionStack.push_back(L);
+L = PLoc.isValid() ? PLoc.getIncludeLoc() : SourceLocation();
+ }
+ if (InclusionStack.size() == 0) {
+// Skip main file
+continue;
+ }
+
+ if (InclusionStack.size() > 1) {
+// Don't care a
[PATCH] D38639: [clangd] #include statements support for Open definition
Nebiroth created this revision.
ctrl-clicking on #include statements now opens the file being pointed by that
statement.
https://reviews.llvm.org/D38639
Files:
clangd/ClangdServer.cpp
clangd/ClangdUnit.cpp
clangd/ClangdUnit.h
unittests/clangd/ClangdTests.cpp
Index: unittests/clangd/ClangdTests.cpp
===
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -7,7 +7,6 @@
//
//===--===//
-#include "ClangdLSPServer.h"
#include "ClangdServer.h"
#include "Logger.h"
#include "clang/Basic/VirtualFileSystem.h"
@@ -900,82 +899,55 @@
}
}
-TEST_F(ClangdVFSTest, CheckSourceHeaderSwitch) {
+TEST_F(ClangdVFSTest, CheckDefinitionIncludes) {
MockFSProvider FS;
ErrorCheckingDiagConsumer DiagConsumer;
MockCompilationDatabase CDB(/*AddFreestandingFlag=*/true);
- ClangdServer Server(CDB, DiagConsumer, FS, getDefaultAsyncThreadsCount(),
+ ClangdServer Server(CDB, DiagConsumer, FS, 0,
/*SnippetCompletions=*/false, EmptyLogger::getInstance());
- auto SourceContents = R"cpp(
+ auto FooCpp = getVirtualTestFilePath("foo.cpp");
+ const auto SourceContents = R"cpp(
#include "foo.h"
+ #include "invalid.h"
int b = a;
)cpp";
-
- auto FooCpp = getVirtualTestFilePath("foo.cpp");
+ FS.Files[FooCpp] = SourceContents;
auto FooH = getVirtualTestFilePath("foo.h");
- auto Invalid = getVirtualTestFilePath("main.cpp");
+ const auto HeaderContents = "int a;";
FS.Files[FooCpp] = SourceContents;
- FS.Files[FooH] = "int a;";
- FS.Files[Invalid] = "int main() { \n return 0; \n }";
+ FS.Files[FooH] = HeaderContents;
- llvm::Optional PathResult = Server.switchSourceHeader(FooCpp);
- EXPECT_TRUE(PathResult.hasValue());
- ASSERT_EQ(PathResult.getValue(), FooH);
-
- PathResult = Server.switchSourceHeader(FooH);
- EXPECT_TRUE(PathResult.hasValue());
- ASSERT_EQ(PathResult.getValue(), FooCpp);
+ Server.addDocument(FooH, HeaderContents);
+ Server.addDocument(FooCpp, SourceContents);
- SourceContents = R"c(
- #include "foo.HH"
- int b = a;
- )c";
+ Position P = Position{1, 11};
- // Test with header file in capital letters and different extension, source
- // file with different extension
- auto FooC = getVirtualTestFilePath("bar.c");
- auto FooHH = getVirtualTestFilePath("bar.HH");
-
- FS.Files[FooC] = SourceContents;
- FS.Files[FooHH] = "int a;";
-
- PathResult = Server.switchSourceHeader(FooC);
- EXPECT_TRUE(PathResult.hasValue());
- ASSERT_EQ(PathResult.getValue(), FooHH);
-
- // Test with both capital letters
- auto Foo2C = getVirtualTestFilePath("foo2.C");
- auto Foo2HH = getVirtualTestFilePath("foo2.HH");
- FS.Files[Foo2C] = SourceContents;
- FS.Files[Foo2HH] = "int a;";
-
- PathResult = Server.switchSourceHeader(Foo2C);
- EXPECT_TRUE(PathResult.hasValue());
- ASSERT_EQ(PathResult.getValue(), Foo2HH);
-
- // Test with source file as capital letter and .hxx header file
- auto Foo3C = getVirtualTestFilePath("foo3.C");
- auto Foo3HXX = getVirtualTestFilePath("foo3.hxx");
+ std::vector Locations = (Server.findDefinitions(FooCpp, P)).Value;
+ EXPECT_TRUE(!Locations.empty());
+ std::string s("file:///");
+ std::string check = URI::unparse(Locations[0].uri);
+ check = check.erase(0, s.size());
+ check = check.substr(0, check.size() - 1);
+ ASSERT_EQ(check, FooH);
+ ASSERT_EQ(Locations[0].range.start.line, 0);
+ ASSERT_EQ(Locations[0].range.start.character, 0);
+ ASSERT_EQ(Locations[0].range.end.line, 0);
+ ASSERT_EQ(Locations[0].range.end.character, 0);
+
+ // Test ctrl-clicking on the #include part on the statement
+ Position P3 = Position{1, 3};
- SourceContents = R"c(
- #include "foo3.hxx"
- int b = a;
- )c";
+ Locations = (Server.findDefinitions(FooCpp, P3)).Value;
+ EXPECT_TRUE(!Locations.empty());
- FS.Files[Foo3C] = SourceContents;
- FS.Files[Foo3HXX] = "int a;";
+ // Test invalid include
+ Position P2 = Position{2, 11};
- PathResult = Server.switchSourceHeader(Foo3C);
- EXPECT_TRUE(PathResult.hasValue());
- ASSERT_EQ(PathResult.getValue(), Foo3HXX);
-
- // Test if asking for a corresponding file that doesn't exist returns an empty
- // string.
- PathResult = Server.switchSourceHeader(Invalid);
- EXPECT_FALSE(PathResult.hasValue());
+ Locations = (Server.findDefinitions(FooCpp, P2)).Value;
+ EXPECT_TRUE(Locations.empty());
}
TEST_F(ClangdThreadingTest, NoConcurrentDiagnostics) {
Index: clangd/ClangdUnit.h
===
--- clangd/ClangdUnit.h
+++ clangd/ClangdUnit.h
@@ -127,11 +127,13 @@
struct PreambleData {
PreambleData(PrecompiledPreamble Preamble,
std::vector TopLevelDeclIDs,
- std::vector Diags);
+ std::vector Diags,
+ std::map IncludeMap);
PrecompiledPreamble Preamble;
std::vector TopLevelDeclIDs;
std::
