[PATCH] D46835: [ASTImporter] Do not try to remove invisible Decls from DeclContext
martong closed this revision. martong added a comment. Closed by commit: https://reviews.llvm.org/rL332699 Repository: rC Clang https://reviews.llvm.org/D46835 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46835: [ASTImporter] Do not try to remove invisible Decls from DeclContext
aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM Repository: rC Clang https://reviews.llvm.org/D46835 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46835: [ASTImporter] Do not try to remove invisible Decls from DeclContext
martong updated this revision to Diff 147274.
martong added a comment.
Addressing review comments.
Repository:
rC Clang
https://reviews.llvm.org/D46835
Files:
lib/AST/DeclBase.cpp
unittests/AST/ASTImporterTest.cpp
Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -1797,5 +1797,38 @@
compoundStmt(has(callExpr(has(unresolvedMemberExpr());
}
+struct DeclContextTest : ASTImporterTestBase {};
+
+TEST_P(DeclContextTest, removeDeclOfClassTemplateSpecialization) {
+ Decl *TU = getTuDecl(
+ R"(
+ namespace NS {
+
+ template
+ struct S {};
+ template struct S;
+
+ inline namespace INS {
+template
+struct S {};
+template struct S;
+ }
+
+ }
+ )", Lang_CXX11, "input0.cc");
+ auto *NS = FirstDeclMatcher().match(
+ TU, namespaceDecl());
+ auto *Spec = FirstDeclMatcher().match(
+ TU, classTemplateSpecializationDecl());
+ ASSERT_TRUE(NS->containsDecl(Spec));
+
+ NS->removeDecl(Spec);
+ EXPECT_FALSE(NS->containsDecl(Spec));
+}
+
+INSTANTIATE_TEST_CASE_P(
+ParameterizedTests, DeclContextTest,
+::testing::Values(ArgVector(), ArgVector{"-fdelayed-template-parsing"}),);
+
} // end namespace ast_matchers
} // end namespace clang
Index: lib/AST/DeclBase.cpp
===
--- lib/AST/DeclBase.cpp
+++ lib/AST/DeclBase.cpp
@@ -1350,6 +1350,32 @@
(D->NextInContextAndBits.getPointer() || D == LastDecl));
}
+/// shouldBeHidden - Determine whether a declaration which was declared
+/// within its semantic context should be invisible to qualified name lookup.
+static bool shouldBeHidden(NamedDecl *D) {
+ // Skip unnamed declarations.
+ if (!D->getDeclName())
+return true;
+
+ // Skip entities that can't be found by name lookup into a particular
+ // context.
+ if ((D->getIdentifierNamespace() == 0 && !isa(D)) ||
+ D->isTemplateParameter())
+return true;
+
+ // Skip template specializations.
+ // FIXME: This feels like a hack. Should DeclarationName support
+ // template-ids, or is there a better way to keep specializations
+ // from being visible?
+ if (isa(D))
+return true;
+ if (auto *FD = dyn_cast(D))
+if (FD->isFunctionTemplateSpecialization())
+ return true;
+
+ return false;
+}
+
void DeclContext::removeDecl(Decl *D) {
assert(D->getLexicalDeclContext() == this &&
"decl being removed from non-lexical context");
@@ -1372,16 +1398,22 @@
}
}
}
-
+
// Mark that D is no longer in the decl chain.
D->NextInContextAndBits.setPointer(nullptr);
// Remove D from the lookup table if necessary.
if (isa(D)) {
auto *ND = cast(D);
+// Do not try to remove the declaration if that is invisible to qualified
+// lookup. E.g. template specializations are skipped.
+if (shouldBeHidden(ND))
+ return;
+
// Remove only decls that have a name
-if (!ND->getDeclName()) return;
+if (!ND->getDeclName())
+ return;
auto *DC = D->getDeclContext();
do {
@@ -1438,32 +1470,6 @@
makeDeclVisibleInContextWithFlags(ND, true, true);
}
-/// shouldBeHidden - Determine whether a declaration which was declared
-/// within its semantic context should be invisible to qualified name lookup.
-static bool shouldBeHidden(NamedDecl *D) {
- // Skip unnamed declarations.
- if (!D->getDeclName())
-return true;
-
- // Skip entities that can't be found by name lookup into a particular
- // context.
- if ((D->getIdentifierNamespace() == 0 && !isa(D)) ||
- D->isTemplateParameter())
-return true;
-
- // Skip template specializations.
- // FIXME: This feels like a hack. Should DeclarationName support
- // template-ids, or is there a better way to keep specializations
- // from being visible?
- if (isa(D))
-return true;
- if (auto *FD = dyn_cast(D))
-if (FD->isFunctionTemplateSpecialization())
- return true;
-
- return false;
-}
-
/// buildLookup - Build the lookup data structure with all of the
/// declarations in this DeclContext (and any other contexts linked
/// to it or transparent contexts nested within it) and return it.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46835: [ASTImporter] Do not try to remove invisible Decls from DeclContext
aaron.ballman added a comment. Can you roll back the changes in MatchVerifier.h? Phab is telling me that the only changes are to whitespace. Comment at: lib/AST/DeclBase.cpp:1353 +static bool shouldBeHidden(NamedDecl *D); + Rather than make a hanging forward declaration, can you just move the definition of this function here? Comment at: lib/AST/DeclBase.cpp:1387 +// lookup. E.g. template specializations are skipped. +if (shouldBeHidden(ND)) return; + The `return` should be on its own line per our usual conventions. Repository: rC Clang https://reviews.llvm.org/D46835 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46835: [ASTImporter] Do not try to remove invisible Decls from DeclContext
martong updated this revision to Diff 146847.
martong added a comment.
- Remove unrelated CXX14 changes
Repository:
rC Clang
https://reviews.llvm.org/D46835
Files:
lib/AST/DeclBase.cpp
unittests/AST/ASTImporterTest.cpp
unittests/AST/MatchVerifier.h
Index: unittests/AST/MatchVerifier.h
===
--- unittests/AST/MatchVerifier.h
+++ unittests/AST/MatchVerifier.h
@@ -28,7 +28,7 @@
namespace clang {
namespace ast_matchers {
-enum Language {
+enum Language {
Lang_C,
Lang_C89,
Lang_CXX,
Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -1764,5 +1764,38 @@
compoundStmt(has(callExpr(has(unresolvedMemberExpr());
}
+struct DeclContextTest : ASTImporterTestBase {};
+
+TEST_P(DeclContextTest, removeDeclOfClassTemplateSpecialization) {
+ Decl *TU = getTuDecl(
+ R"(
+ namespace NS {
+
+ template
+ struct S {};
+ template struct S;
+
+ inline namespace INS {
+template
+struct S {};
+template struct S;
+ }
+
+ }
+ )", Lang_CXX11, "input0.cc");
+ auto *NS = FirstDeclMatcher().match(
+ TU, namespaceDecl());
+ auto *Spec = FirstDeclMatcher().match(
+ TU, classTemplateSpecializationDecl());
+ ASSERT_TRUE(NS->containsDecl(Spec));
+
+ NS->removeDecl(Spec);
+ EXPECT_FALSE(NS->containsDecl(Spec));
+}
+
+INSTANTIATE_TEST_CASE_P(
+ParameterizedTests, DeclContextTest,
+::testing::Values(ArgVector(), ArgVector{"-fdelayed-template-parsing"}),);
+
} // end namespace ast_matchers
} // end namespace clang
Index: lib/AST/DeclBase.cpp
===
--- lib/AST/DeclBase.cpp
+++ lib/AST/DeclBase.cpp
@@ -1350,6 +1350,8 @@
(D->NextInContextAndBits.getPointer() || D == LastDecl));
}
+static bool shouldBeHidden(NamedDecl *D);
+
void DeclContext::removeDecl(Decl *D) {
assert(D->getLexicalDeclContext() == this &&
"decl being removed from non-lexical context");
@@ -1372,14 +1374,18 @@
}
}
}
-
+
// Mark that D is no longer in the decl chain.
D->NextInContextAndBits.setPointer(nullptr);
// Remove D from the lookup table if necessary.
if (isa(D)) {
auto *ND = cast(D);
+// Do not try to remove the declaration if that is invisible to qualified
+// lookup. E.g. template specializations are skipped.
+if (shouldBeHidden(ND)) return;
+
// Remove only decls that have a name
if (!ND->getDeclName()) return;
Index: unittests/AST/MatchVerifier.h
===
--- unittests/AST/MatchVerifier.h
+++ unittests/AST/MatchVerifier.h
@@ -28,7 +28,7 @@
namespace clang {
namespace ast_matchers {
-enum Language {
+enum Language {
Lang_C,
Lang_C89,
Lang_CXX,
Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -1764,5 +1764,38 @@
compoundStmt(has(callExpr(has(unresolvedMemberExpr());
}
+struct DeclContextTest : ASTImporterTestBase {};
+
+TEST_P(DeclContextTest, removeDeclOfClassTemplateSpecialization) {
+ Decl *TU = getTuDecl(
+ R"(
+ namespace NS {
+
+ template
+ struct S {};
+ template struct S;
+
+ inline namespace INS {
+template
+struct S {};
+template struct S;
+ }
+
+ }
+ )", Lang_CXX11, "input0.cc");
+ auto *NS = FirstDeclMatcher().match(
+ TU, namespaceDecl());
+ auto *Spec = FirstDeclMatcher().match(
+ TU, classTemplateSpecializationDecl());
+ ASSERT_TRUE(NS->containsDecl(Spec));
+
+ NS->removeDecl(Spec);
+ EXPECT_FALSE(NS->containsDecl(Spec));
+}
+
+INSTANTIATE_TEST_CASE_P(
+ParameterizedTests, DeclContextTest,
+::testing::Values(ArgVector(), ArgVector{"-fdelayed-template-parsing"}),);
+
} // end namespace ast_matchers
} // end namespace clang
Index: lib/AST/DeclBase.cpp
===
--- lib/AST/DeclBase.cpp
+++ lib/AST/DeclBase.cpp
@@ -1350,6 +1350,8 @@
(D->NextInContextAndBits.getPointer() || D == LastDecl));
}
+static bool shouldBeHidden(NamedDecl *D);
+
void DeclContext::removeDecl(Decl *D) {
assert(D->getLexicalDeclContext() == this &&
"decl being removed from non-lexical context");
@@ -1372,14 +1374,18 @@
}
}
}
-
+
// Mark that D is no longer in the decl chain.
D->NextInContextAndBits.setPointer(nullptr);
// Remove D from the lookup table if necessary.
if (isa(D)) {
auto *ND = cast(D);
+// Do not try to remove the declaration if that is invisible to qualified
+// lookup. E.g. template specializations are
[PATCH] D46835: [ASTImporter] Do not try to remove invisible Decls from DeclContext
martong updated this revision to Diff 146845.
martong marked 4 inline comments as done.
martong added a comment.
- Add test for removeDecl, remove unrelated tests
Repository:
rC Clang
https://reviews.llvm.org/D46835
Files:
lib/AST/DeclBase.cpp
unittests/AST/ASTImporterTest.cpp
unittests/AST/MatchVerifier.h
Index: unittests/AST/MatchVerifier.h
===
--- unittests/AST/MatchVerifier.h
+++ unittests/AST/MatchVerifier.h
@@ -28,11 +28,12 @@
namespace clang {
namespace ast_matchers {
-enum Language {
+enum Language {
Lang_C,
Lang_C89,
Lang_CXX,
Lang_CXX11,
+Lang_CXX14,
Lang_OpenCL,
Lang_OBJCXX
};
@@ -113,6 +114,10 @@
Args.push_back("-std=c++11");
FileName = "input.cc";
break;
+ case Lang_CXX14:
+Args.push_back("-std=c++14");
+FileName = "input.cc";
+break;
case Lang_OpenCL:
FileName = "input.cl";
break;
Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -52,6 +52,9 @@
case Lang_CXX11:
BasicArgs = {"-std=c++11", "-frtti"};
break;
+ case Lang_CXX14:
+BasicArgs = {"-std=c++14", "-frtti"};
+break;
case Lang_OpenCL:
case Lang_OBJCXX:
llvm_unreachable("Not implemented yet!");
@@ -1764,5 +1767,38 @@
compoundStmt(has(callExpr(has(unresolvedMemberExpr());
}
+struct DeclContextTest : ASTImporterTestBase {};
+
+TEST_P(DeclContextTest, removeDeclOfClassTemplateSpecialization) {
+ Decl *TU = getTuDecl(
+ R"(
+ namespace NS {
+
+ template
+ struct S {};
+ template struct S;
+
+ inline namespace INS {
+template
+struct S {};
+template struct S;
+ }
+
+ }
+ )", Lang_CXX11, "input0.cc");
+ auto *NS = FirstDeclMatcher().match(
+ TU, namespaceDecl());
+ auto *Spec = FirstDeclMatcher().match(
+ TU, classTemplateSpecializationDecl());
+ ASSERT_TRUE(NS->containsDecl(Spec));
+
+ NS->removeDecl(Spec);
+ EXPECT_FALSE(NS->containsDecl(Spec));
+}
+
+INSTANTIATE_TEST_CASE_P(
+ParameterizedTests, DeclContextTest,
+::testing::Values(ArgVector(), ArgVector{"-fdelayed-template-parsing"}),);
+
} // end namespace ast_matchers
} // end namespace clang
Index: lib/AST/DeclBase.cpp
===
--- lib/AST/DeclBase.cpp
+++ lib/AST/DeclBase.cpp
@@ -1350,6 +1350,8 @@
(D->NextInContextAndBits.getPointer() || D == LastDecl));
}
+static bool shouldBeHidden(NamedDecl *D);
+
void DeclContext::removeDecl(Decl *D) {
assert(D->getLexicalDeclContext() == this &&
"decl being removed from non-lexical context");
@@ -1372,14 +1374,18 @@
}
}
}
-
+
// Mark that D is no longer in the decl chain.
D->NextInContextAndBits.setPointer(nullptr);
// Remove D from the lookup table if necessary.
if (isa(D)) {
auto *ND = cast(D);
+// Do not try to remove the declaration if that is invisible to qualified
+// lookup. E.g. template specializations are skipped.
+if (shouldBeHidden(ND)) return;
+
// Remove only decls that have a name
if (!ND->getDeclName()) return;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46835: [ASTImporter] Do not try to remove invisible Decls from DeclContext
martong added a comment.
Hi Aleksei,
Thanks for reviewing this.
I could synthesize a test which exercises only the `DeclContext::removeDecl`
function. This test causes an assertion without the fix.
Removed the rest of the testcases, which are not strictly connected to this
change.
Comment at: unittests/AST/ASTImporterTest.cpp:1827
+
+TEST_P(ASTImporterTestBase, DISABLED_ImportOfRecordWithDifferentFriends) {
+ Decl *ToR1;
a.sidorin wrote:
> For this change, we should create a separate patch.
This test is disabled ATM, but I agree it would be better to bring this in when
we fix the import of friends.
Repository:
rC Clang
https://reviews.llvm.org/D46835
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46835: [ASTImporter] Do not try to remove invisible Decls from DeclContext
a.sidorin added a comment.
Hi Gabor,
I don't feel I'm a right person to review AST-related part so I'm adding other
reviewers.
What I'm worrying about is that there is no test to check if our changes in
removeDecl are correct. Maybe https://reviews.llvm.org/D44100 is a right patch
for this but we should land it first or set dependencies properly.
Regarding ASTImporter, you can find my comments inline.
Comment at: lib/AST/DeclBase.cpp:1386
+// Do not try to remove the declaration if that is invisible to qualified
+// lookup. E.g. template sepcializations are skipped.
+if (shouldBeHidden(ND)) return;
specializations
Comment at: unittests/AST/ASTImporterTest.cpp:1770
+TEST(ImportExpr, ImportClassTemplatePartialSpecialization) {
+ MatchVerifier Verifier;
These tests seem to be for ImportDecl, not for ImportExpr.
Comment at: unittests/AST/ASTImporterTest.cpp:1803
+
+ testImport(Code, Lang_CXX11, "", Lang_CXX11, Verifier, namespaceDecl());
+}
Check for namespaceDecl() looks too weak because import of NamespaceDecl
succeeds even if import of its nested decls fails. Same below.
Comment at: unittests/AST/ASTImporterTest.cpp:1827
+
+TEST_P(ASTImporterTestBase, DISABLED_ImportOfRecordWithDifferentFriends) {
+ Decl *ToR1;
For this change, we should create a separate patch.
Repository:
rC Clang
https://reviews.llvm.org/D46835
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46835: [ASTImporter] Do not try to remove invisible Decls from DeclContext
martong created this revision.
martong added reviewers: xazax.hun, a.sidorin.
Herald added subscribers: cfe-commits, dkrupp, rnkovacs.
`DeclContext` is essentially a list of `Decl`s and a lookup table (`LookupPtr`)
but these are encapsulated.
E.g. `LookupPtr` is private. `DeclContext::removeDecl` has the responsibility
to remove the given decl from the list and from the table too.
Template specializations cannot participate in normal (qualified) lookup.
Consequently no template specialization can be in the lookup table, but they
will be in the list. When the lookup table is built or when a new `Decl` is
added, then it is checked whether it is a template specialization and if so it
is skipped from the lookup table.
Thus, whenever we want to remove a `Decl` from a `DeclContext` we must not
reach the point to remove that from the lookup table (and that is what this
patch do).
With respect to ASTImporter: At some point probably we will be reordering
`FriendDecl`s and possibly `CXXMethodDecl`s in a `RecordDecl` at
`importDeclContext` to be able to structurally compare two `RecordDecl`s.
When that happens we most probably want to remove all `Decls` from a
`RecordDecl` and then we would add them in the proper order.
Repository:
rC Clang
https://reviews.llvm.org/D46835
Files:
lib/AST/DeclBase.cpp
unittests/AST/ASTImporterTest.cpp
unittests/AST/MatchVerifier.h
Index: unittests/AST/MatchVerifier.h
===
--- unittests/AST/MatchVerifier.h
+++ unittests/AST/MatchVerifier.h
@@ -28,11 +28,12 @@
namespace clang {
namespace ast_matchers {
-enum Language {
+enum Language {
Lang_C,
Lang_C89,
Lang_CXX,
Lang_CXX11,
+Lang_CXX14,
Lang_OpenCL,
Lang_OBJCXX
};
@@ -113,6 +114,10 @@
Args.push_back("-std=c++11");
FileName = "input.cc";
break;
+ case Lang_CXX14:
+Args.push_back("-std=c++14");
+FileName = "input.cc";
+break;
case Lang_OpenCL:
FileName = "input.cl";
break;
Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -52,6 +52,9 @@
case Lang_CXX11:
BasicArgs = {"-std=c++11", "-frtti"};
break;
+ case Lang_CXX14:
+BasicArgs = {"-std=c++14", "-frtti"};
+break;
case Lang_OpenCL:
case Lang_OBJCXX:
llvm_unreachable("Not implemented yet!");
@@ -1764,5 +1767,86 @@
compoundStmt(has(callExpr(has(unresolvedMemberExpr());
}
+TEST(ImportExpr, ImportClassTemplatePartialSpecialization) {
+ MatchVerifier Verifier;
+ auto Code =
+R"s(
+struct declToImport {
+ template
+ struct X {};
+ template
+ struct X {};
+ X x0;
+};
+)s";
+
+ testImport(Code, Lang_CXX, "", Lang_CXX, Verifier, recordDecl());
+}
+
+TEST(ImportExpr, ImportSpecializationsOfFriendClassTemplate) {
+ MatchVerifier Verifier;
+ auto Code =
+R"(
+namespace declToImport {
+
+template
+struct F{};
+
+class X {
+ friend struct F;
+ friend struct F;
+};
+
+} // namespace
+)";
+
+ testImport(Code, Lang_CXX11, "", Lang_CXX11, Verifier, namespaceDecl());
+}
+
+TEST(ImportExpr, ImportSpecializationsOfVariableTemplate) {
+ MatchVerifier Verifier;
+ auto Code =
+R"(
+namespace declToImport {
+
+struct limits {
+ template
+ static const T min; // declaration of a static data member template
+};
+template
+const T limits::min = { }; // definition of a static data member template
+
+template const int limits::min; // explicit instantiation
+
+} // namespace
+)";
+
+ testImport(Code, Lang_CXX14, "", Lang_CXX14, Verifier, namespaceDecl());
+}
+
+TEST_P(ASTImporterTestBase, DISABLED_ImportOfRecordWithDifferentFriends) {
+ Decl *ToR1;
+ {
+Decl *FromTU = getTuDecl("struct A { friend class F0; friend class F1; };",
+ Lang_CXX, "input0.cc");
+auto *FromR = FirstDeclMatcher().match(
+FromTU, cxxRecordDecl(hasName("A")));
+
+ToR1 = Import(FromR, Lang_CXX);
+ }
+
+ Decl *ToR2;
+ {
+Decl *FromTU =
+getTuDecl("struct A { friend class F0; };", Lang_CXX, "input1.cc");
+auto *FromR = FirstDeclMatcher().match(
+FromTU, cxxRecordDecl(hasName("A")));
+
+ToR2 = Import(FromR, Lang_CXX);
+ }
+
+ EXPECT_NE(ToR1, ToR2);
+}
+
} // end namespace ast_matchers
} // end namespace clang
Index: lib/AST/DeclBase.cpp
===
--- lib/AST/DeclBase.cpp
+++ lib/AST/DeclBase.cpp
@@ -1350,6 +1350,8 @@
(D->NextInContextAndBits.getPointer() || D == LastDecl));
}
+static bool shouldBeHidden(NamedDecl *D);
+
void DeclContext::removeDecl(Decl *D) {
assert(D->getLexicalDeclContext() == this &&
"decl being removed from non-lexical context");
@@ -1372,14 +1374,18
