[clang] 4becf68 - [ASTImporter] Friend class decl should not be visible in its context
Gabor Marton via cfe-commits
cfe-commits at lists.llvm.org
Tue Dec 17 05:51:11 PST 2019
Author: Gabor Marton
Date: 2019-12-17T14:48:55+01:00
New Revision: 4becf68c6f17fe143539ceac954b21175914e1c1
URL: https://github.com/llvm/llvm-project/commit/4becf68c6f17fe143539ceac954b21175914e1c1
DIFF: https://github.com/llvm/llvm-project/commit/4becf68c6f17fe143539ceac954b21175914e1c1.diff
LOG: [ASTImporter] Friend class decl should not be visible in its context
Summary:
In the past we had to use DeclContext::makeDeclVisibleInContext to make
friend declarations available for subsequent lookup calls and this way
we could chain (redecl) the structurally equivalent decls.
By doing this we created an AST that improperly made declarations
visible in some contexts, so the AST was malformed.
Since we use the importer specific lookup this is no longer necessary,
because with that we can find every previous nodes.
Reviewers: balazske, a_sidorin, a.sidorin, shafik
Subscribers: rnkovacs, dkrupp, Szelethus, gamesh411, teemperor, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D71020
Added:
Modified:
clang/lib/AST/ASTImporter.cpp
clang/unittests/AST/ASTImporterTest.cpp
Removed:
################################################################################
diff --git a/clang/lib/AST/ASTImporter.cpp b/clang/lib/AST/ASTImporter.cpp
index 414092f33c47..336ee88f994a 100644
--- a/clang/lib/AST/ASTImporter.cpp
+++ b/clang/lib/AST/ASTImporter.cpp
@@ -298,6 +298,45 @@ namespace clang {
return nullptr;
}
+ void addDeclToContexts(Decl *FromD, Decl *ToD) {
+ if (Importer.isMinimalImport()) {
+ // In minimal import case the decl must be added even if it is not
+ // contained in original context, for LLDB compatibility.
+ // FIXME: Check if a better solution is possible.
+ if (!FromD->getDescribedTemplate() &&
+ FromD->getFriendObjectKind() == Decl::FOK_None)
+ ToD->getLexicalDeclContext()->addDeclInternal(ToD);
+ return;
+ }
+
+ DeclContext *FromDC = FromD->getDeclContext();
+ DeclContext *FromLexicalDC = FromD->getLexicalDeclContext();
+ DeclContext *ToDC = ToD->getDeclContext();
+ DeclContext *ToLexicalDC = ToD->getLexicalDeclContext();
+
+ bool Visible = false;
+ if (FromDC->containsDeclAndLoad(FromD)) {
+ ToDC->addDeclInternal(ToD);
+ Visible = true;
+ }
+ if (ToDC != ToLexicalDC && FromLexicalDC->containsDeclAndLoad(FromD)) {
+ ToLexicalDC->addDeclInternal(ToD);
+ Visible = true;
+ }
+
+ // If the Decl was added to any context, it was made already visible.
+ // Otherwise it is still possible that it should be visible.
+ if (!Visible) {
+ if (auto *FromNamed = dyn_cast<NamedDecl>(FromD)) {
+ auto *ToNamed = cast<NamedDecl>(ToD);
+ DeclContextLookupResult FromLookup =
+ FromDC->lookup(FromNamed->getDeclName());
+ if (llvm::is_contained(FromLookup, FromNamed))
+ ToDC->makeDeclVisibleInContext(ToNamed);
+ }
+ }
+ }
+
public:
explicit ASTNodeImporter(ASTImporter &Importer) : Importer(Importer) {}
@@ -2737,11 +2776,7 @@ ExpectedDecl ASTNodeImporter::VisitRecordDecl(RecordDecl *D) {
D2 = D2CXX;
D2->setAccess(D->getAccess());
D2->setLexicalDeclContext(LexicalDC);
- if (!DCXX->getDescribedClassTemplate() || DCXX->isImplicit())
- LexicalDC->addDeclInternal(D2);
-
- if (LexicalDC != DC && D->isInIdentifierNamespace(Decl::IDNS_TagFriend))
- DC->makeDeclVisibleInContext(D2);
+ addDeclToContexts(D, D2);
if (ClassTemplateDecl *FromDescribed =
DCXX->getDescribedClassTemplate()) {
@@ -2807,7 +2842,7 @@ ExpectedDecl ASTNodeImporter::VisitRecordDecl(RecordDecl *D) {
Name.getAsIdentifierInfo(), PrevDecl))
return D2;
D2->setLexicalDeclContext(LexicalDC);
- LexicalDC->addDeclInternal(D2);
+ addDeclToContexts(D, D2);
}
if (auto BraceRangeOrErr = import(D->getBraceRange()))
@@ -3386,23 +3421,7 @@ ExpectedDecl ASTNodeImporter::VisitFunctionDecl(FunctionDecl *D) {
if (Error Err = ImportTemplateInformation(D, ToFunction))
return std::move(Err);
- bool IsFriend = D->isInIdentifierNamespace(Decl::IDNS_OrdinaryFriend);
-
- // TODO Can we generalize this approach to other AST nodes as well?
- if (D->getDeclContext()->containsDeclAndLoad(D))
- DC->addDeclInternal(ToFunction);
- if (DC != LexicalDC && D->getLexicalDeclContext()->containsDeclAndLoad(D))
- LexicalDC->addDeclInternal(ToFunction);
-
- // Friend declaration's lexical context is the befriending class, but the
- // semantic context is the enclosing scope of the befriending class.
- // We want the friend functions to be found in the semantic context by lookup.
- // FIXME should we handle this generically in VisitFriendDecl?
- // In Other cases when LexicalDC != DC we don't want it to be added,
- // e.g out-of-class definitions like void B::f() {} .
- if (LexicalDC != DC && IsFriend) {
- DC->makeDeclVisibleInContext(ToFunction);
- }
+ addDeclToContexts(D, ToFunction);
if (auto *FromCXXMethod = dyn_cast<CXXMethodDecl>(D))
if (Error Err = ImportOverriddenMethods(cast<CXXMethodDecl>(ToFunction),
@@ -3850,10 +3869,7 @@ ExpectedDecl ASTNodeImporter::VisitVarDecl(VarDecl *D) {
if (D->isConstexpr())
ToVar->setConstexpr(true);
- if (D->getDeclContext()->containsDeclAndLoad(D))
- DC->addDeclInternal(ToVar);
- if (DC != LexicalDC && D->getLexicalDeclContext()->containsDeclAndLoad(D))
- LexicalDC->addDeclInternal(ToVar);
+ addDeclToContexts(D, ToVar);
// Import the rest of the chain. I.e. import all subsequent declarations.
for (++RedeclIt; RedeclIt != Redecls.end(); ++RedeclIt) {
@@ -5133,7 +5149,6 @@ template <typename T> static auto getTemplateDefinition(T *D) -> T * {
}
ExpectedDecl ASTNodeImporter::VisitClassTemplateDecl(ClassTemplateDecl *D) {
- bool IsFriend = D->getFriendObjectKind() != Decl::FOK_None;
// Import the major distinguishing characteristics of this class template.
DeclContext *DC, *LexicalDC;
@@ -5210,10 +5225,7 @@ ExpectedDecl ASTNodeImporter::VisitClassTemplateDecl(ClassTemplateDecl *D) {
D2->setAccess(D->getAccess());
D2->setLexicalDeclContext(LexicalDC);
- if (D->getDeclContext()->containsDeclAndLoad(D))
- DC->addDeclInternal(D2);
- if (DC != LexicalDC && D->getLexicalDeclContext()->containsDeclAndLoad(D))
- LexicalDC->addDeclInternal(D2);
+ addDeclToContexts(D, D2);
if (FoundByLookup) {
auto *Recent =
@@ -5239,9 +5251,6 @@ ExpectedDecl ASTNodeImporter::VisitClassTemplateDecl(ClassTemplateDecl *D) {
D2->setPreviousDecl(Recent);
}
- if (LexicalDC != DC && IsFriend)
- DC->makeDeclVisibleInContext(D2);
-
if (FromTemplated->isCompleteDefinition() &&
!ToTemplated->isCompleteDefinition()) {
// FIXME: Import definition!
diff --git a/clang/unittests/AST/ASTImporterTest.cpp b/clang/unittests/AST/ASTImporterTest.cpp
index 6652111fd48e..3e8f804374f4 100644
--- a/clang/unittests/AST/ASTImporterTest.cpp
+++ b/clang/unittests/AST/ASTImporterTest.cpp
@@ -247,6 +247,11 @@ template <typename T> RecordDecl *getRecordDecl(T *D) {
return cast<RecordType>(ET->getNamedType().getTypePtr())->getDecl();
}
+static const RecordDecl *getRecordDeclOfFriend(FriendDecl *FD) {
+ QualType Ty = FD->getFriendType()->getType().getCanonicalType();
+ return cast<RecordType>(Ty)->getDecl();
+}
+
struct ImportExpr : TestImportBase {};
struct ImportType : TestImportBase {};
struct ImportDecl : TestImportBase {};
@@ -2707,7 +2712,7 @@ TEST_P(ImportFriendFunctions, Lookup) {
EXPECT_FALSE(To0->isInIdentifierNamespace(Decl::IDNS_Ordinary));
}
-TEST_P(ImportFriendFunctions, DISABLED_LookupWithProtoAfter) {
+TEST_P(ImportFriendFunctions, LookupWithProtoAfter) {
auto FunctionPattern = functionDecl(hasName("f"));
auto ClassPattern = cxxRecordDecl(hasName("X"));
@@ -3778,6 +3783,44 @@ TEST_P(ImportFriendClasses, ImportOfRecursiveFriendClass) {
EXPECT_TRUE(MatchVerifier<Decl>{}.match(ToD, Pattern));
}
+TEST_P(ImportFriendClasses, UndeclaredFriendClassShouldNotBeVisible) {
+ Decl *FromTu = getTuDecl("class X { friend class Y; };", Lang_CXX, "from.cc");
+ auto *FromX = FirstDeclMatcher<CXXRecordDecl>().match(
+ FromTu, cxxRecordDecl(hasName("X")));
+ auto *FromFriend = FirstDeclMatcher<FriendDecl>().match(FromTu, friendDecl());
+ RecordDecl *FromRecordOfFriend =
+ const_cast<RecordDecl *>(getRecordDeclOfFriend(FromFriend));
+
+ ASSERT_EQ(FromRecordOfFriend->getDeclContext(), cast<DeclContext>(FromTu));
+ ASSERT_EQ(FromRecordOfFriend->getLexicalDeclContext(),
+ cast<DeclContext>(FromX));
+ ASSERT_FALSE(
+ FromRecordOfFriend->getDeclContext()->containsDecl(FromRecordOfFriend));
+ ASSERT_FALSE(FromRecordOfFriend->getLexicalDeclContext()->containsDecl(
+ FromRecordOfFriend));
+ ASSERT_FALSE(FromRecordOfFriend->getLookupParent()
+ ->lookup(FromRecordOfFriend->getDeclName())
+ .empty());
+
+ auto *ToX = Import(FromX, Lang_CXX);
+ ASSERT_TRUE(ToX);
+
+ Decl *ToTu = ToX->getTranslationUnitDecl();
+ auto *ToFriend = FirstDeclMatcher<FriendDecl>().match(ToTu, friendDecl());
+ RecordDecl *ToRecordOfFriend =
+ const_cast<RecordDecl *>(getRecordDeclOfFriend(ToFriend));
+
+ ASSERT_EQ(ToRecordOfFriend->getDeclContext(), cast<DeclContext>(ToTu));
+ ASSERT_EQ(ToRecordOfFriend->getLexicalDeclContext(), cast<DeclContext>(ToX));
+ EXPECT_FALSE(
+ ToRecordOfFriend->getDeclContext()->containsDecl(ToRecordOfFriend));
+ EXPECT_FALSE(ToRecordOfFriend->getLexicalDeclContext()->containsDecl(
+ ToRecordOfFriend));
+ EXPECT_FALSE(ToRecordOfFriend->getLookupParent()
+ ->lookup(ToRecordOfFriend->getDeclName())
+ .empty());
+}
+
TEST_P(ImportFriendClasses, ImportOfRecursiveFriendClassTemplate) {
Decl *FromTu = getTuDecl(
R"(
@@ -4477,11 +4520,6 @@ TEST_P(ASTImporterLookupTableTest, LookupDeclNamesFromDifferentTUs) {
ASSERT_EQ(Res.size(), 0u);
}
-static const RecordDecl *getRecordDeclOfFriend(FriendDecl *FD) {
- QualType Ty = FD->getFriendType()->getType().getCanonicalType();
- return cast<RecordType>(Ty)->getDecl();
-}
-
TEST_P(ASTImporterLookupTableTest,
LookupFindsFwdFriendClassDeclWithElaboratedType) {
TranslationUnitDecl *ToTU = getToTuDecl(
More information about the cfe-commits
mailing list