[PATCH] D63817: [clangd] No longer getting template instantiations from header files in Main AST.
This revision was automatically updated to reflect the committed changes.
Closed by commit rL364747: [clangd] No longer getting template instantiations
from header files in Main… (authored by jvikstrom, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D63817?vs=206685=207261#toc
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63817/new/
https://reviews.llvm.org/D63817
Files:
clang-tools-extra/trunk/clangd/ClangdUnit.cpp
clang-tools-extra/trunk/clangd/unittests/ClangdUnitTests.cpp
Index: clang-tools-extra/trunk/clangd/ClangdUnit.cpp
===
--- clang-tools-extra/trunk/clangd/ClangdUnit.cpp
+++ clang-tools-extra/trunk/clangd/ClangdUnit.cpp
@@ -67,7 +67,8 @@
bool HandleTopLevelDecl(DeclGroupRef DG) override {
for (Decl *D : DG) {
- if (D->isFromASTFile())
+ auto = D->getASTContext().getSourceManager();
+ if (!SM.isWrittenInMainFile(SM.getExpansionLoc(D->getLocation(
continue;
// ObjCMethodDecl are not actually top-level decls.
Index: clang-tools-extra/trunk/clangd/unittests/ClangdUnitTests.cpp
===
--- clang-tools-extra/trunk/clangd/unittests/ClangdUnitTests.cpp
+++ clang-tools-extra/trunk/clangd/unittests/ClangdUnitTests.cpp
@@ -83,6 +83,26 @@
EXPECT_THAT(AST.getLocalTopLevelDecls(), ElementsAre(DeclNamed("main")));
}
+TEST(ClangdUnitTest, DoesNotGetIncludedTopDecls) {
+ TestTU TU;
+ TU.HeaderCode = R"cpp(
+#define LL void foo(){}
+template
+struct H {
+ H() {}
+ LL
+};
+ )cpp";
+ TU.Code = R"cpp(
+int main() {
+ H h;
+ h.foo();
+}
+ )cpp";
+ auto AST = TU.build();
+ EXPECT_THAT(AST.getLocalTopLevelDecls(), ElementsAre(DeclNamed("main")));
+}
+
TEST(ClangdUnitTest, TokensAfterPreamble) {
TestTU TU;
TU.AdditionalFiles["foo.h"] = R"(
Index: clang-tools-extra/trunk/clangd/ClangdUnit.cpp
===
--- clang-tools-extra/trunk/clangd/ClangdUnit.cpp
+++ clang-tools-extra/trunk/clangd/ClangdUnit.cpp
@@ -67,7 +67,8 @@
bool HandleTopLevelDecl(DeclGroupRef DG) override {
for (Decl *D : DG) {
- if (D->isFromASTFile())
+ auto = D->getASTContext().getSourceManager();
+ if (!SM.isWrittenInMainFile(SM.getExpansionLoc(D->getLocation(
continue;
// ObjCMethodDecl are not actually top-level decls.
Index: clang-tools-extra/trunk/clangd/unittests/ClangdUnitTests.cpp
===
--- clang-tools-extra/trunk/clangd/unittests/ClangdUnitTests.cpp
+++ clang-tools-extra/trunk/clangd/unittests/ClangdUnitTests.cpp
@@ -83,6 +83,26 @@
EXPECT_THAT(AST.getLocalTopLevelDecls(), ElementsAre(DeclNamed("main")));
}
+TEST(ClangdUnitTest, DoesNotGetIncludedTopDecls) {
+ TestTU TU;
+ TU.HeaderCode = R"cpp(
+#define LL void foo(){}
+template
+struct H {
+ H() {}
+ LL
+};
+ )cpp";
+ TU.Code = R"cpp(
+int main() {
+ H h;
+ h.foo();
+}
+ )cpp";
+ auto AST = TU.build();
+ EXPECT_THAT(AST.getLocalTopLevelDecls(), ElementsAre(DeclNamed("main")));
+}
+
TEST(ClangdUnitTest, TokensAfterPreamble) {
TestTU TU;
TU.AdditionalFiles["foo.h"] = R"(
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D63817: [clangd] No longer getting template instantiations from header files in Main AST.
hokein accepted this revision.
hokein added a comment.
This revision is now accepted and ready to land.
Looks good, please update the patch description (in git, and phabricator),
mentioning why the previous solution won't work.
Comment at: clang-tools-extra/clangd/ClangdUnit.cpp:71
+ auto = D->getASTContext().getSourceManager();
+ if (!SM.isWrittenInMainFile(SM.getExpansionLoc(D->getLocation( {
continue;
nit: remove the `{}`
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63817/new/
https://reviews.llvm.org/D63817
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D63817: [clangd] No longer getting template instantiations from header files in Main AST.
ilya-biryukov added inline comments.
Comment at: clang-tools-extra/clangd/ClangdUnit.cpp:47
#include
+#include
NIT: This include is redundant, maybe remove?
Probably added by accident.
Comment at: clang-tools-extra/clangd/ClangdUnit.cpp:71
for (Decl *D : DG) {
- if (D->isFromASTFile())
+ if
(!D->getASTContext().getSourceManager().isInMainFile(D->getLocation()))
continue;
Please use `isWrittenInMainFile` instead.
`isInMainFile` takes `#line` markers into account, we don't want that in clangd.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63817/new/
https://reviews.llvm.org/D63817
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D63817: [clangd] No longer getting template instantiations from header files in Main AST.
jvikstrom marked 3 inline comments as done. jvikstrom added inline comments. Comment at: clang-tools-extra/clangd/ClangdUnit.cpp:75 + if (!SM->isInMainFile(D->getLocation())) +// This decl comes from another file and should not be included in the +// top level decls. hokein wrote: > nit: This comment just repeats the code, I think the comment describe why we > need to do this check (because of the template instantiation?) Removed the D->isFromASTFile because this if actually covers that as well. Don't know what to do about comments though as it is not only specific to that case. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63817/new/ https://reviews.llvm.org/D63817 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D63817: [clangd] No longer getting template instantiations from header files in Main AST.
jvikstrom updated this revision to Diff 206664.
jvikstrom added a comment.
Removed comment, getting SM from Decl and removed old check for checking the
file.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63817/new/
https://reviews.llvm.org/D63817
Files:
clang-tools-extra/clangd/ClangdUnit.cpp
clang-tools-extra/clangd/unittests/ClangdUnitTests.cpp
Index: clang-tools-extra/clangd/unittests/ClangdUnitTests.cpp
===
--- clang-tools-extra/clangd/unittests/ClangdUnitTests.cpp
+++ clang-tools-extra/clangd/unittests/ClangdUnitTests.cpp
@@ -83,6 +83,23 @@
EXPECT_THAT(AST.getLocalTopLevelDecls(), ElementsAre(DeclNamed("main")));
}
+TEST(ClangdUnitTest, DoesNotGetIncludedTopDecls) {
+ TestTU TU;
+ TU.HeaderCode = R"cpp(
+template
+struct H {
+ H() {}
+};
+ )cpp";
+ TU.Code = R"cpp(
+int main() {
+ H h;
+}
+ )cpp";
+ auto AST = TU.build();
+ EXPECT_THAT(AST.getLocalTopLevelDecls(), ElementsAre(DeclNamed("main")));
+}
+
TEST(ClangdUnitTest, TokensAfterPreamble) {
TestTU TU;
TU.AdditionalFiles["foo.h"] = R"(
Index: clang-tools-extra/clangd/ClangdUnit.cpp
===
--- clang-tools-extra/clangd/ClangdUnit.cpp
+++ clang-tools-extra/clangd/ClangdUnit.cpp
@@ -44,6 +44,7 @@
#include "llvm/Support/raw_ostream.h"
#include
#include
+#include
namespace clang {
namespace clangd {
@@ -67,7 +68,7 @@
bool HandleTopLevelDecl(DeclGroupRef DG) override {
for (Decl *D : DG) {
- if (D->isFromASTFile())
+ if
(!D->getASTContext().getSourceManager().isInMainFile(D->getLocation()))
continue;
// ObjCMethodDecl are not actually top-level decls.
Index: clang-tools-extra/clangd/unittests/ClangdUnitTests.cpp
===
--- clang-tools-extra/clangd/unittests/ClangdUnitTests.cpp
+++ clang-tools-extra/clangd/unittests/ClangdUnitTests.cpp
@@ -83,6 +83,23 @@
EXPECT_THAT(AST.getLocalTopLevelDecls(), ElementsAre(DeclNamed("main")));
}
+TEST(ClangdUnitTest, DoesNotGetIncludedTopDecls) {
+ TestTU TU;
+ TU.HeaderCode = R"cpp(
+template
+struct H {
+ H() {}
+};
+ )cpp";
+ TU.Code = R"cpp(
+int main() {
+ H h;
+}
+ )cpp";
+ auto AST = TU.build();
+ EXPECT_THAT(AST.getLocalTopLevelDecls(), ElementsAre(DeclNamed("main")));
+}
+
TEST(ClangdUnitTest, TokensAfterPreamble) {
TestTU TU;
TU.AdditionalFiles["foo.h"] = R"(
Index: clang-tools-extra/clangd/ClangdUnit.cpp
===
--- clang-tools-extra/clangd/ClangdUnit.cpp
+++ clang-tools-extra/clangd/ClangdUnit.cpp
@@ -44,6 +44,7 @@
#include "llvm/Support/raw_ostream.h"
#include
#include
+#include
namespace clang {
namespace clangd {
@@ -67,7 +68,7 @@
bool HandleTopLevelDecl(DeclGroupRef DG) override {
for (Decl *D : DG) {
- if (D->isFromASTFile())
+ if (!D->getASTContext().getSourceManager().isInMainFile(D->getLocation()))
continue;
// ObjCMethodDecl are not actually top-level decls.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D63817: [clangd] No longer getting template instantiations from header files in Main AST.
hokein added inline comments. Comment at: clang-tools-extra/clangd/ClangdUnit.cpp:74 + if (!SM->isInMainFile(D->getLocation())) +// This decl comes from another file and should not be included in the you could get SourceManager from `D->getASTContext().getSourceManager()`. Comment at: clang-tools-extra/clangd/ClangdUnit.cpp:75 + if (!SM->isInMainFile(D->getLocation())) +// This decl comes from another file and should not be included in the +// top level decls. nit: This comment just repeats the code, I think the comment describe why we need to do this check (because of the template instantiation?) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63817/new/ https://reviews.llvm.org/D63817 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
