[PATCH] D47632: [ASTImporter] Refactor Decl creation
martong added a comment. > I apologize for the delay in reviewing patches. There is no need to apologize. On the contrary, we (me and my colleges at Ericsson) would like to thank you for the effort you had put into reviewing these patches. This period was hard, we provided so many patches lately, because we had the time to open source a lot of our work only from May. We still have a few minor patches, but all the major patches are in the llvm tree already (If you are interested, here you can track which patches we still plan to create: https://github.com/Ericsson/clang/projects/2 ). We always received very useful comments from you, which I think greatly increased the quality of the patches. Thanks again. Repository: rL LLVM https://reviews.llvm.org/D47632 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47632: [ASTImporter] Refactor Decl creation
This revision was automatically updated to reflect the committed changes.
Closed by commit rL336896: [ASTImporter] Refactor Decl creation (authored by
martong, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D47632?vs=154582=155134#toc
Repository:
rL LLVM
https://reviews.llvm.org/D47632
Files:
cfe/trunk/include/clang/AST/ASTImporter.h
cfe/trunk/include/clang/AST/ASTStructuralEquivalence.h
cfe/trunk/include/clang/AST/DeclBase.h
cfe/trunk/lib/AST/ASTImporter.cpp
cfe/trunk/lib/AST/ASTStructuralEquivalence.cpp
cfe/trunk/lib/AST/ExternalASTMerger.cpp
cfe/trunk/lib/Sema/SemaType.cpp
cfe/trunk/unittests/AST/ASTImporterTest.cpp
cfe/trunk/unittests/AST/StructuralEquivalenceTest.cpp
Index: cfe/trunk/lib/AST/ASTImporter.cpp
===
--- cfe/trunk/lib/AST/ASTImporter.cpp
+++ cfe/trunk/lib/AST/ASTImporter.cpp
@@ -90,11 +90,83 @@
return getCanonicalForwardRedeclChain(FD);
}
+ void updateFlags(const Decl *From, Decl *To) {
+// Check if some flags or attrs are new in 'From' and copy into 'To'.
+// FIXME: Other flags or attrs?
+if (From->isUsed(false) && !To->isUsed(false))
+ To->setIsUsed();
+ }
+
class ASTNodeImporter : public TypeVisitor,
public DeclVisitor,
public StmtVisitor {
ASTImporter
+// Wrapper for an overload set.
+template struct CallOverloadedCreateFun {
+ template
+ auto operator()(Args &&... args)
+ -> decltype(ToDeclT::Create(std::forward(args)...)) {
+return ToDeclT::Create(std::forward(args)...);
+ }
+};
+
+// Always use these functions to create a Decl during import. There are
+// certain tasks which must be done after the Decl was created, e.g. we
+// must immediately register that as an imported Decl. The parameter `ToD`
+// will be set to the newly created Decl or if had been imported before
+// then to the already imported Decl. Returns a bool value set to true if
+// the `FromD` had been imported before.
+template
+LLVM_NODISCARD bool GetImportedOrCreateDecl(ToDeclT *, FromDeclT *FromD,
+Args &&... args) {
+ // There may be several overloads of ToDeclT::Create. We must make sure
+ // to call the one which would be chosen by the arguments, thus we use a
+ // wrapper for the overload set.
+ CallOverloadedCreateFun OC;
+ return GetImportedOrCreateSpecialDecl(ToD, OC, FromD,
+std::forward(args)...);
+}
+// Use this overload if a special Type is needed to be created. E.g if we
+// want to create a `TypeAliasDecl` and assign that to a `TypedefNameDecl`
+// then:
+// TypedefNameDecl *ToTypedef;
+// GetImportedOrCreateDecl(ToTypedef, FromD, ...);
+template
+LLVM_NODISCARD bool GetImportedOrCreateDecl(ToDeclT *, FromDeclT *FromD,
+Args &&... args) {
+ CallOverloadedCreateFun OC;
+ return GetImportedOrCreateSpecialDecl(ToD, OC, FromD,
+std::forward(args)...);
+}
+// Use this version if a special create function must be
+// used, e.g. CXXRecordDecl::CreateLambda .
+template
+LLVM_NODISCARD bool
+GetImportedOrCreateSpecialDecl(ToDeclT *, CreateFunT CreateFun,
+ FromDeclT *FromD, Args &&... args) {
+ ToD = cast_or_null(Importer.GetAlreadyImportedOrNull(FromD));
+ if (ToD)
+return true; // Already imported.
+ ToD = CreateFun(std::forward(args)...);
+ InitializeImportedDecl(FromD, ToD);
+ return false; // A new Decl is created.
+}
+
+void InitializeImportedDecl(Decl *FromD, Decl *ToD) {
+ Importer.MapImported(FromD, ToD);
+ ToD->IdentifierNamespace = FromD->IdentifierNamespace;
+ if (FromD->hasAttrs())
+for (const Attr *FromAttr : FromD->getAttrs())
+ ToD->addAttr(Importer.Import(FromAttr));
+ if (FromD->isUsed())
+ToD->setIsUsed();
+ if (FromD->isImplicit())
+ToD->setImplicit();
+}
+
public:
explicit ASTNodeImporter(ASTImporter ) : Importer(Importer) {}
@@ -1485,6 +1557,12 @@
return false;
}
+static StructuralEquivalenceKind
+getStructuralEquivalenceKind(const ASTImporter ) {
+ return Importer.isMinimalImport() ? StructuralEquivalenceKind::Minimal
+: StructuralEquivalenceKind::Default;
+}
+
bool ASTNodeImporter::IsStructuralMatch(RecordDecl *FromRecord,
RecordDecl *ToRecord, bool Complain) {
// Eliminate a potential failure point where we attempt to re-import
@@ -1499,37 +1577,41 @@
StructuralEquivalenceContext Ctx(Importer.getFromContext(),
[PATCH] D47632: [ASTImporter] Refactor Decl creation
a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. Thank you Gabor! I apologize for the delay in reviewing patches. Repository: rC Clang https://reviews.llvm.org/D47632 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47632: [ASTImporter] Refactor Decl creation
martong added a comment. Hi Aleksei, could you pleas take an other quick look, there were only minor changes since your last comments. Thanks! Repository: rC Clang https://reviews.llvm.org/D47632 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47632: [ASTImporter] Refactor Decl creation
martong updated this revision to Diff 154582.
martong marked 6 inline comments as done.
martong added a comment.
Address review comments
Repository:
rC Clang
https://reviews.llvm.org/D47632
Files:
include/clang/AST/ASTImporter.h
include/clang/AST/ASTStructuralEquivalence.h
include/clang/AST/DeclBase.h
lib/AST/ASTImporter.cpp
lib/AST/ASTStructuralEquivalence.cpp
lib/AST/ExternalASTMerger.cpp
lib/Sema/SemaType.cpp
unittests/AST/ASTImporterTest.cpp
unittests/AST/StructuralEquivalenceTest.cpp
Index: unittests/AST/StructuralEquivalenceTest.cpp
===
--- unittests/AST/StructuralEquivalenceTest.cpp
+++ unittests/AST/StructuralEquivalenceTest.cpp
@@ -60,8 +60,9 @@
bool testStructuralMatch(NamedDecl *D0, NamedDecl *D1) {
llvm::DenseSet> NonEquivalentDecls;
-StructuralEquivalenceContext Ctx(D0->getASTContext(), D1->getASTContext(),
- NonEquivalentDecls, false, false);
+StructuralEquivalenceContext Ctx(
+D0->getASTContext(), D1->getASTContext(), NonEquivalentDecls,
+StructuralEquivalenceKind::Default, false, false);
return Ctx.IsStructurallyEquivalent(D0, D1);
}
Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -284,19 +284,32 @@
// Buffer for the To context, must live in the test scope.
std::string ToCode;
+ // Represents a "From" translation unit and holds an importer object which we
+ // use to import from this translation unit.
struct TU {
// Buffer for the context, must live in the test scope.
std::string Code;
std::string FileName;
std::unique_ptr Unit;
TranslationUnitDecl *TUDecl = nullptr;
+std::unique_ptr Importer;
TU(StringRef Code, StringRef FileName, ArgVector Args)
: Code(Code), FileName(FileName),
Unit(tooling::buildASTFromCodeWithArgs(this->Code, Args,
this->FileName)),
TUDecl(Unit->getASTContext().getTranslationUnitDecl()) {
Unit->enableSourceFileDiagnostics();
}
+
+Decl *import(ASTUnit *ToAST, Decl *FromDecl) {
+ assert(ToAST);
+ if (!Importer) {
+Importer.reset(new ASTImporter(
+ToAST->getASTContext(), ToAST->getFileManager(),
+Unit->getASTContext(), Unit->getFileManager(), false));
+ }
+ return Importer->Import(FromDecl);
+}
};
// We may have several From contexts and related translation units. In each
@@ -329,14 +342,10 @@
ToAST = tooling::buildASTFromCodeWithArgs(ToCode, ToArgs, OutputFileName);
ToAST->enableSourceFileDiagnostics();
-ASTContext = FromTU.Unit->getASTContext(),
-= ToAST->getASTContext();
+ASTContext = FromTU.Unit->getASTContext();
createVirtualFileIfNeeded(ToAST.get(), InputFileName, FromTU.Code);
-ASTImporter Importer(ToCtx, ToAST->getFileManager(), FromCtx,
- FromTU.Unit->getFileManager(), false);
-
IdentifierInfo *ImportedII = (Identifier);
assert(ImportedII && "Declaration with the given identifier "
"should be specified in test!");
@@ -347,7 +356,8 @@
assert(FoundDecls.size() == 1);
-Decl *Imported = Importer.Import(FoundDecls.front());
+Decl *Imported = FromTU.import(ToAST.get(), FoundDecls.front());
+
assert(Imported);
return std::make_tuple(*FoundDecls.begin(), Imported);
}
@@ -401,11 +411,7 @@
assert(It != FromTUs.end());
createVirtualFileIfNeeded(ToAST.get(), It->FileName, It->Code);
-ASTContext = From->getASTContext(),
-= ToAST->getASTContext();
-ASTImporter Importer(ToCtx, ToAST->getFileManager(), FromCtx,
- FromCtx.getSourceManager().getFileManager(), false);
-return Importer.Import(From);
+return It->import(ToAST.get(), From);
}
~ASTImporterTestBase() {
@@ -1089,8 +1095,7 @@
EXPECT_EQ(ToTemplated1, ToTemplated);
}
-TEST_P(ASTImporterTestBase,
- DISABLED_ImportOfTemplatedDeclOfFunctionTemplateDecl) {
+TEST_P(ASTImporterTestBase, ImportOfTemplatedDeclOfFunctionTemplateDecl) {
Decl *FromTU = getTuDecl("template void f(){}", Lang_CXX);
auto From = FirstDeclMatcher().match(
FromTU, functionTemplateDecl());
@@ -1166,7 +1171,7 @@
ASSERT_EQ(ToTemplated1, ToTemplated);
}
-TEST_P(ASTImporterTestBase, DISABLED_ImportFunctionWithBackReferringParameter) {
+TEST_P(ASTImporterTestBase, ImportFunctionWithBackReferringParameter) {
Decl *From, *To;
std::tie(From, To) = getImportedDecl(
R"(
@@ -1711,6 +1716,49 @@
EXPECT_NE(To0->getCanonicalDecl(), To1->getCanonicalDecl());
}
+TEST_P(ASTImporterTestBase, ImportDoesUpdateUsedFlag) {
+ auto Pattern = varDecl(hasName("x"));
+ VarDecl *Imported1;
+ {
+Decl *FromTU =
[PATCH] D47632: [ASTImporter] Refactor Decl creation
martong added inline comments. Comment at: lib/AST/ASTImporter.cpp:161 + ToD->IdentifierNamespace = FromD->IdentifierNamespace; + if (FromD->hasAttrs()) +for (const Attr *FromAttr : FromD->getAttrs()) a_sidorin wrote: > Should we move the below operations into `updateAttrAndFlags()` and use it > instead? `updateAttrAndFlags` should handle only those properties of a `Decl` which can change during a subsequent import. Such as `isUsed` and `isReferenced`. There are other properties which cannot change, e.g. `isImplicit`. Similarly, `Decl::hasAttrs()` refers to the syntactic attributes of a declaration, e.g. `[[noreturn]]`, which will not change during a subsequent import. Perhaps the `Attr` syllable is confusing in the `updateAttrAndFlags()` function. Thus I suggest a new name: `updateFlags()`. Comment at: lib/AST/ASTImporter.cpp:1587 StructuralEquivalenceContext Ctx( Importer.getFromContext(), Importer.getToContext(), + Importer.getNonEquivalentDecls(), getStructuralEquivalenceKind(Importer), a_sidorin wrote: > (Thinking out loud) We need to introduce some method to return > `StructuralEquivalenceContext` in ASTImporter. But this is an item for a > separate patch, not this one. Yes, I agree. Or perhaps we should have a common `isStructuralMatch(Decl*, Decl*)` function which is called by the other overloads of `isStructuralMatch`. Comment at: lib/AST/ASTImporter.cpp:1890 TypedefNameDecl *ToTypedef; - if (IsAlias) -ToTypedef = TypeAliasDecl::Create(Importer.getToContext(), DC, StartL, Loc, - Name.getAsIdentifierInfo(), TInfo); - else -ToTypedef = TypedefDecl::Create(Importer.getToContext(), DC, -StartL, Loc, -Name.getAsIdentifierInfo(), -TInfo); + if (IsAlias && GetImportedOrCreateDecl( + ToTypedef, D, Importer.getToContext(), DC, StartL, Loc, a_sidorin wrote: > This is not strictly equivalent to the source condition. If > GetImportedOrCreateDecl returns false, we'll fall to the `else` branch, and > it doesn't seem correct to me. Thanks, this is a very good catch. Fixed it. Comment at: lib/AST/ASTImporter.cpp:6905 Decl *ToD = Pos->second; +// FIXME: remove this call from this function ASTNodeImporter(*this).ImportDefinitionIfNeeded(FromD, ToD); balazske wrote: > I think this comment is not needed (or with other text). There is a case when > `GetAlreadyImportedOrNull` is called during import of a Decl that is already > imported but its definition is not yet completely imported. If this call is > here we have after `GetAlreadyImportedOrNull` a Decl with complete > definition. (Name of the function is still bad: It does not only "get" but > makes update too. The `ImportDefinitionIfNeeded` call can be moved into the > decl create function?) Yes, I agree. Changed the text of the code, because I believe that we should do the import of the definition on the call side of `GetAlreadyImportedOrNull` for the case where it is needed (in `ImportDeclParts`). Repository: rC Clang https://reviews.llvm.org/D47632 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47632: [ASTImporter] Refactor Decl creation
balazske added inline comments. Comment at: lib/AST/ASTImporter.cpp:6905 Decl *ToD = Pos->second; +// FIXME: remove this call from this function ASTNodeImporter(*this).ImportDefinitionIfNeeded(FromD, ToD); I think this comment is not needed (or with other text). There is a case when `GetAlreadyImportedOrNull` is called during import of a Decl that is already imported but its definition is not yet completely imported. If this call is here we have after `GetAlreadyImportedOrNull` a Decl with complete definition. (Name of the function is still bad: It does not only "get" but makes update too. The `ImportDefinitionIfNeeded` call can be moved into the decl create function?) Repository: rC Clang https://reviews.llvm.org/D47632 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47632: [ASTImporter] Refactor Decl creation
a_sidorin added a comment.
Hi Gabor,
I like the new syntax. There are some comments inline; most of them are just
stylish.
Comment at: lib/AST/ASTImporter.cpp:94
+ void updateAttrAndFlags(const Decl *From, Decl *To) {
+// check if some flags or attrs are new in 'From' and copy into 'To'
+// FIXME: other flags or attrs?
Comments should be complete sentences: start with a capital and end with a
period.
Comment at: lib/AST/ASTImporter.cpp:114
+
+// Always use theses functions to create a Decl during import. There are
+// certain tasks which must be done after the Decl was created, e.g. we
these?
Comment at: lib/AST/ASTImporter.cpp:161
+ ToD->IdentifierNamespace = FromD->IdentifierNamespace;
+ if (FromD->hasAttrs())
+for (const Attr *FromAttr : FromD->getAttrs())
Should we move the below operations into `updateAttrAndFlags()` and use it
instead?
Comment at: lib/AST/ASTImporter.cpp:1587
StructuralEquivalenceContext Ctx(
Importer.getFromContext(), Importer.getToContext(),
+ Importer.getNonEquivalentDecls(), getStructuralEquivalenceKind(Importer),
(Thinking out loud) We need to introduce some method to return
`StructuralEquivalenceContext` in ASTImporter. But this is an item for a
separate patch, not this one.
Comment at: lib/AST/ASTImporter.cpp:1890
TypedefNameDecl *ToTypedef;
- if (IsAlias)
-ToTypedef = TypeAliasDecl::Create(Importer.getToContext(), DC, StartL, Loc,
- Name.getAsIdentifierInfo(), TInfo);
- else
-ToTypedef = TypedefDecl::Create(Importer.getToContext(), DC,
-StartL, Loc,
-Name.getAsIdentifierInfo(),
-TInfo);
+ if (IsAlias && GetImportedOrCreateDecl(
+ ToTypedef, D, Importer.getToContext(), DC, StartL, Loc,
This is not strictly equivalent to the source condition. If
GetImportedOrCreateDecl returns false, we'll fall to the `else` branch, and it
doesn't seem correct to me.
Comment at: lib/AST/ASTImporter.cpp:4274
+ TemplateParams))
+return ToD;
+ return ToD;
Can we just ignore the return value by casting it to void here and in similar
cases?
Comment at: lib/AST/ASTStructuralEquivalence.cpp:895
+ // If any of the records has external storage and we do a minimal check (or
+ // AST import) we assmue they are equivalent. (If we didn't have this
+ // assumption then `RecordDecl::LoadFieldsFromExternalStorage` could trigger
assume
Repository:
rC Clang
https://reviews.llvm.org/D47632
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47632: [ASTImporter] Refactor Decl creation
martong updated this revision to Diff 154421.
martong added a comment.
Fix indentation
Repository:
rC Clang
https://reviews.llvm.org/D47632
Files:
include/clang/AST/ASTImporter.h
include/clang/AST/ASTStructuralEquivalence.h
include/clang/AST/DeclBase.h
lib/AST/ASTImporter.cpp
lib/AST/ASTStructuralEquivalence.cpp
lib/AST/ExternalASTMerger.cpp
lib/Sema/SemaType.cpp
unittests/AST/ASTImporterTest.cpp
unittests/AST/StructuralEquivalenceTest.cpp
Index: unittests/AST/StructuralEquivalenceTest.cpp
===
--- unittests/AST/StructuralEquivalenceTest.cpp
+++ unittests/AST/StructuralEquivalenceTest.cpp
@@ -60,8 +60,9 @@
bool testStructuralMatch(NamedDecl *D0, NamedDecl *D1) {
llvm::DenseSet> NonEquivalentDecls;
-StructuralEquivalenceContext Ctx(D0->getASTContext(), D1->getASTContext(),
- NonEquivalentDecls, false, false);
+StructuralEquivalenceContext Ctx(
+D0->getASTContext(), D1->getASTContext(), NonEquivalentDecls,
+StructuralEquivalenceKind::Default, false, false);
return Ctx.IsStructurallyEquivalent(D0, D1);
}
Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -284,19 +284,32 @@
// Buffer for the To context, must live in the test scope.
std::string ToCode;
+ // Represents a "From" translation unit and holds an importer object which we
+ // use to import from this translation unit.
struct TU {
// Buffer for the context, must live in the test scope.
std::string Code;
std::string FileName;
std::unique_ptr Unit;
TranslationUnitDecl *TUDecl = nullptr;
+std::unique_ptr Importer;
TU(StringRef Code, StringRef FileName, ArgVector Args)
: Code(Code), FileName(FileName),
Unit(tooling::buildASTFromCodeWithArgs(this->Code, Args,
this->FileName)),
TUDecl(Unit->getASTContext().getTranslationUnitDecl()) {
Unit->enableSourceFileDiagnostics();
}
+
+Decl *import(ASTUnit *ToAST, Decl *FromDecl) {
+ assert(ToAST);
+ if (!Importer) {
+Importer.reset(new ASTImporter(
+ToAST->getASTContext(), ToAST->getFileManager(),
+Unit->getASTContext(), Unit->getFileManager(), false));
+ }
+ return Importer->Import(FromDecl);
+}
};
// We may have several From contexts and related translation units. In each
@@ -329,14 +342,10 @@
ToAST = tooling::buildASTFromCodeWithArgs(ToCode, ToArgs, OutputFileName);
ToAST->enableSourceFileDiagnostics();
-ASTContext = FromTU.Unit->getASTContext(),
-= ToAST->getASTContext();
+ASTContext = FromTU.Unit->getASTContext();
createVirtualFileIfNeeded(ToAST.get(), InputFileName, FromTU.Code);
-ASTImporter Importer(ToCtx, ToAST->getFileManager(), FromCtx,
- FromTU.Unit->getFileManager(), false);
-
IdentifierInfo *ImportedII = (Identifier);
assert(ImportedII && "Declaration with the given identifier "
"should be specified in test!");
@@ -347,7 +356,8 @@
assert(FoundDecls.size() == 1);
-Decl *Imported = Importer.Import(FoundDecls.front());
+Decl *Imported = FromTU.import(ToAST.get(), FoundDecls.front());
+
assert(Imported);
return std::make_tuple(*FoundDecls.begin(), Imported);
}
@@ -401,11 +411,7 @@
assert(It != FromTUs.end());
createVirtualFileIfNeeded(ToAST.get(), It->FileName, It->Code);
-ASTContext = From->getASTContext(),
-= ToAST->getASTContext();
-ASTImporter Importer(ToCtx, ToAST->getFileManager(), FromCtx,
- FromCtx.getSourceManager().getFileManager(), false);
-return Importer.Import(From);
+return It->import(ToAST.get(), From);
}
~ASTImporterTestBase() {
@@ -1089,8 +1095,7 @@
EXPECT_EQ(ToTemplated1, ToTemplated);
}
-TEST_P(ASTImporterTestBase,
- DISABLED_ImportOfTemplatedDeclOfFunctionTemplateDecl) {
+TEST_P(ASTImporterTestBase, ImportOfTemplatedDeclOfFunctionTemplateDecl) {
Decl *FromTU = getTuDecl("template void f(){}", Lang_CXX);
auto From = FirstDeclMatcher().match(
FromTU, functionTemplateDecl());
@@ -1166,7 +1171,7 @@
ASSERT_EQ(ToTemplated1, ToTemplated);
}
-TEST_P(ASTImporterTestBase, DISABLED_ImportFunctionWithBackReferringParameter) {
+TEST_P(ASTImporterTestBase, ImportFunctionWithBackReferringParameter) {
Decl *From, *To;
std::tie(From, To) = getImportedDecl(
R"(
@@ -1711,6 +1716,49 @@
EXPECT_NE(To0->getCanonicalDecl(), To1->getCanonicalDecl());
}
+TEST_P(ASTImporterTestBase, ImportDoesUpdateUsedFlag) {
+ auto Pattern = varDecl(hasName("x"));
+ VarDecl *Imported1;
+ {
+Decl *FromTU = getTuDecl("extern int x;", Lang_CXX, "input0.cc");
[PATCH] D47632: [ASTImporter] Refactor Decl creation
martong updated this revision to Diff 154418.
martong added a comment.
- Rebase from master
- Enable some disabled tests
- Update the isUsed flag
- Add a new config `Minimal` to the structural eq check, so lldb tests can pass
now
- Return with a bool value and use LLVM_NODISCARD in CreateDecl
- Rename CreateDecl
Repository:
rC Clang
https://reviews.llvm.org/D47632
Files:
include/clang/AST/ASTImporter.h
include/clang/AST/ASTStructuralEquivalence.h
include/clang/AST/DeclBase.h
lib/AST/ASTImporter.cpp
lib/AST/ASTStructuralEquivalence.cpp
lib/AST/ExternalASTMerger.cpp
lib/Sema/SemaType.cpp
unittests/AST/ASTImporterTest.cpp
unittests/AST/StructuralEquivalenceTest.cpp
Index: unittests/AST/StructuralEquivalenceTest.cpp
===
--- unittests/AST/StructuralEquivalenceTest.cpp
+++ unittests/AST/StructuralEquivalenceTest.cpp
@@ -60,8 +60,9 @@
bool testStructuralMatch(NamedDecl *D0, NamedDecl *D1) {
llvm::DenseSet> NonEquivalentDecls;
-StructuralEquivalenceContext Ctx(D0->getASTContext(), D1->getASTContext(),
- NonEquivalentDecls, false, false);
+StructuralEquivalenceContext Ctx(
+D0->getASTContext(), D1->getASTContext(), NonEquivalentDecls,
+StructuralEquivalenceKind::Default, false, false);
return Ctx.IsStructurallyEquivalent(D0, D1);
}
Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -284,19 +284,32 @@
// Buffer for the To context, must live in the test scope.
std::string ToCode;
+ // Represents a "From" translation unit and holds an importer object which we
+ // use to import from this translation unit.
struct TU {
// Buffer for the context, must live in the test scope.
std::string Code;
std::string FileName;
std::unique_ptr Unit;
TranslationUnitDecl *TUDecl = nullptr;
+std::unique_ptr Importer;
TU(StringRef Code, StringRef FileName, ArgVector Args)
: Code(Code), FileName(FileName),
Unit(tooling::buildASTFromCodeWithArgs(this->Code, Args,
this->FileName)),
TUDecl(Unit->getASTContext().getTranslationUnitDecl()) {
Unit->enableSourceFileDiagnostics();
}
+
+Decl *import(ASTUnit *ToAST, Decl *FromDecl) {
+ assert(ToAST);
+ if (!Importer) {
+Importer.reset(new ASTImporter(
+ToAST->getASTContext(), ToAST->getFileManager(),
+Unit->getASTContext(), Unit->getFileManager(), false));
+ }
+ return Importer->Import(FromDecl);
+}
};
// We may have several From contexts and related translation units. In each
@@ -329,14 +342,10 @@
ToAST = tooling::buildASTFromCodeWithArgs(ToCode, ToArgs, OutputFileName);
ToAST->enableSourceFileDiagnostics();
-ASTContext = FromTU.Unit->getASTContext(),
-= ToAST->getASTContext();
+ASTContext = FromTU.Unit->getASTContext();
createVirtualFileIfNeeded(ToAST.get(), InputFileName, FromTU.Code);
-ASTImporter Importer(ToCtx, ToAST->getFileManager(), FromCtx,
- FromTU.Unit->getFileManager(), false);
-
IdentifierInfo *ImportedII = (Identifier);
assert(ImportedII && "Declaration with the given identifier "
"should be specified in test!");
@@ -347,7 +356,8 @@
assert(FoundDecls.size() == 1);
-Decl *Imported = Importer.Import(FoundDecls.front());
+Decl *Imported = FromTU.import(ToAST.get(), FoundDecls.front());
+
assert(Imported);
return std::make_tuple(*FoundDecls.begin(), Imported);
}
@@ -401,11 +411,7 @@
assert(It != FromTUs.end());
createVirtualFileIfNeeded(ToAST.get(), It->FileName, It->Code);
-ASTContext = From->getASTContext(),
-= ToAST->getASTContext();
-ASTImporter Importer(ToCtx, ToAST->getFileManager(), FromCtx,
- FromCtx.getSourceManager().getFileManager(), false);
-return Importer.Import(From);
+return It->import(ToAST.get(), From);
}
~ASTImporterTestBase() {
@@ -1089,8 +1095,7 @@
EXPECT_EQ(ToTemplated1, ToTemplated);
}
-TEST_P(ASTImporterTestBase,
- DISABLED_ImportOfTemplatedDeclOfFunctionTemplateDecl) {
+TEST_P(ASTImporterTestBase, ImportOfTemplatedDeclOfFunctionTemplateDecl) {
Decl *FromTU = getTuDecl("template void f(){}", Lang_CXX);
auto From = FirstDeclMatcher().match(
FromTU, functionTemplateDecl());
@@ -1166,7 +1171,7 @@
ASSERT_EQ(ToTemplated1, ToTemplated);
}
-TEST_P(ASTImporterTestBase, DISABLED_ImportFunctionWithBackReferringParameter) {
+TEST_P(ASTImporterTestBase, ImportFunctionWithBackReferringParameter) {
Decl *From, *To;
std::tie(From, To) = getImportedDecl(
R"(
@@ -1711,6 +1716,49 @@
EXPECT_NE(To0->getCanonicalDecl(),
[PATCH] D47632: [ASTImporter] Refactor Decl creation
martong added a comment. I realized that, this patch brakes 3 lldb tests ATM: - `TestTopLevelExprs.py`. If https://reviews.llvm.org/D48722 was merged then this test would not be broken. - `TestPersistentTypes.py` If https://reviews.llvm.org/D48773 was merged then this test would not be broken. - `TestRecursiveTypes.py`. I am still working on this. The newly introduced assert fires: `Assertion `(Pos == ImportedDecls.end() || Pos->second == To) && "Try to import an already imported Decl"' failed.`. Repository: rC Clang https://reviews.llvm.org/D47632 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47632: [ASTImporter] Refactor Decl creation
a.sidorin added a comment. Hi Gabor, Let's agree on `getImportedOrCreateDecl()` :) I think it is informative enough but is still not too long. Repository: rC Clang https://reviews.llvm.org/D47632 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47632: [ASTImporter] Refactor Decl creation
martong added inline comments. Comment at: lib/AST/ASTImporter.cpp:1659 + AccessSpecDecl *ToD; + std::tie(ToD, AlreadyImported) = CreateDecl( + D, Importer.getToContext(), D->getAccess(), DC, Loc, ColonLoc); a.sidorin wrote: > As I see, all usage samples require having a variable AlreadyImported used > only once. How about changing the signature a bit: > ```bool getOrCreateDecl(ToDeclTy *, FromDeclT *FromD, Args &&... args)``` > with optional `LLVM_NODISCARD`? (Naming is a subject for discussion). > This signature also allows us to omit template arguments because we pass an > argument of a templated type. So, the call will look like this: > ```AccessSpecDecl *ToD; > if (getOrCreateDecl(ToD, D, Importer.getToContext(), D->getAccess(), DC, Loc, > ColonLoc)) > return ToD;``` I like your idea, because indeed we could spare the repetition of the type in most cases. However, in one particular case we have to use the original version: in `VisitTypedefNameDecl` where we assign either a `TypeAliasDecl *` or a `TypedefDecl *` to a pointer to their common base class, `TypedefNameDecl`. So, I think it is possible to add an overload with the signature `(ToDeclTy *, FromDeclT *FromD, Args &&... args)` and we could use that in the simple cases (which is the majority). About the naming I was thinking about `getAlreadyImportedOrCreateNewDecl`, but this appears to be very long. So if you think this is way too long then I am OK with the shorter `getOrCreateDecl`. Repository: rC Clang https://reviews.llvm.org/D47632 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47632: [ASTImporter] Refactor Decl creation
a.sidorin added a comment. Hi Gabor! I like the change but there are also some questions. Comment at: lib/AST/ASTImporter.cpp:1659 + AccessSpecDecl *ToD; + std::tie(ToD, AlreadyImported) = CreateDecl( + D, Importer.getToContext(), D->getAccess(), DC, Loc, ColonLoc); As I see, all usage samples require having a variable AlreadyImported used only once. How about changing the signature a bit: ```bool getOrCreateDecl(ToDeclTy *, FromDeclT *FromD, Args &&... args)``` with optional `LLVM_NODISCARD`? (Naming is a subject for discussion). This signature also allows us to omit template arguments because we pass an argument of a templated type. So, the call will look like this: ```AccessSpecDecl *ToD; if (getOrCreateDecl(ToD, D, Importer.getToContext(), D->getAccess(), DC, Loc, ColonLoc)) return ToD;``` Comment at: lib/AST/ASTImporter.cpp:1922 if (auto *FoundAlias = dyn_cast(FoundDecl)) - return Importer.Imported(D, FoundAlias); + return Importer.MapImported(D, FoundAlias); ConflictingDecls.push_back(FoundDecl); Could you also fix indentation here? Repository: rC Clang https://reviews.llvm.org/D47632 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47632: [ASTImporter] Refactor Decl creation
martong updated this revision to Diff 152703.
martong added a comment.
- Rebase from master.
Repository:
rC Clang
https://reviews.llvm.org/D47632
Files:
include/clang/AST/ASTImporter.h
include/clang/AST/DeclBase.h
lib/AST/ASTImporter.cpp
lib/AST/ExternalASTMerger.cpp
Index: lib/AST/ExternalASTMerger.cpp
===
--- lib/AST/ExternalASTMerger.cpp
+++ lib/AST/ExternalASTMerger.cpp
@@ -154,7 +154,7 @@
ToContainer->setMustBuildLookupTable();
assert(Parent.CanComplete(ToContainer));
}
-return ASTImporter::Imported(From, To);
+return To;
}
ASTImporter () { return Reverse; }
};
@@ -229,7 +229,7 @@
SourceTag->getASTContext().getExternalSource()->CompleteType(SourceTag);
if (!SourceTag->getDefinition())
return false;
-Forward.Imported(SourceTag, Tag);
+Forward.MapImported(SourceTag, Tag);
Forward.ImportDefinition(SourceTag);
Tag->setCompleteDefinition(SourceTag->isCompleteDefinition());
return true;
@@ -248,7 +248,7 @@
SourceInterface);
if (!SourceInterface->getDefinition())
return false;
-Forward.Imported(SourceInterface, Interface);
+Forward.MapImported(SourceInterface, Interface);
Forward.ImportDefinition(SourceInterface);
return true;
});
@@ -304,7 +304,7 @@
void ExternalASTMerger::RecordOriginImpl(const DeclContext *ToDC, DCOrigin Origin,
ASTImporter ) {
Origins[ToDC] = Origin;
- Importer.ASTImporter::Imported(cast(Origin.DC), const_cast(cast(ToDC)));
+ Importer.ASTImporter::MapImported(cast(Origin.DC), const_cast(cast(ToDC)));
}
ExternalASTMerger::ExternalASTMerger(const ImporterTarget ,
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -95,6 +95,58 @@
public StmtVisitor {
ASTImporter
+// Wrapper for an overload set.
+template struct CallOverloadedCreateFun {
+ template
+ auto operator()(Args &&... args)
+ -> decltype(ToDeclT::Create(std::forward(args)...)) {
+return ToDeclT::Create(std::forward(args)...);
+ }
+};
+
+// Always use this function to create a Decl during import. There are
+// certain tasks which must be done after the Decl was created, e.g. we
+// must immediately register that as an imported Decl.
+// Returns a pair consisting of a pointer to the new or the already imported
+// Decl and a bool value set to true if the `FromD` had been imported
+// before.
+template
+std::pair CreateDecl(FromDeclT *FromD, Args &&... args) {
+ // There may be several overloads of ToDeclT::Create. We must make sure
+ // to call the one which would be chosen by the arguments, thus we use a
+ // wrapper for the overload set.
+ CallOverloadedCreateFun OC;
+ return CreateDecl(OC, FromD, std::forward(args)...);
+}
+// Use this overload directly only if a special create function must be
+// used, e.g. CXXRecordDecl::CreateLambda .
+template
+auto CreateDecl(CreateFunT CreateFun, FromDeclT *FromD, Args &&... args)
+-> std::pair(args)...)), bool> {
+ using ToDeclT = typename std::remove_pointer(args)...))>::type;
+ ToDeclT *AlreadyImported =
+ cast_or_null(Importer.GetAlreadyImportedOrNull(FromD));
+ if (AlreadyImported) {
+return std::make_pair(AlreadyImported, /*AlreadyImported=*/true);
+ }
+ ToDeclT *ToD = CreateFun(std::forward(args)...);
+ InitializeImportedDecl(FromD, ToD);
+ return std::make_pair(ToD, /*AlreadyImported=*/false);
+}
+
+void InitializeImportedDecl(Decl *FromD, Decl *ToD) {
+ Importer.MapImported(FromD, ToD);
+ ToD->IdentifierNamespace = FromD->IdentifierNamespace;
+ if (FromD->hasAttrs())
+for (const Attr *FromAttr : FromD->getAttrs())
+ ToD->addAttr(Importer.Import(FromAttr));
+ if (FromD->isUsed())
+ToD->setIsUsed();
+ if (FromD->isImplicit())
+ToD->setImplicit();
+}
+
public:
explicit ASTNodeImporter(ASTImporter ) : Importer(Importer) {}
@@ -1572,18 +1624,23 @@
// Import the location of this declaration.
SourceLocation Loc = Importer.Import(D->getLocation());
- EmptyDecl *ToD = EmptyDecl::Create(Importer.getToContext(), DC, Loc);
+ bool AlreadyImported;
+ EmptyDecl *ToD;
+ std::tie(ToD, AlreadyImported) =
+ CreateDecl(D, Importer.getToContext(), DC, Loc);
+ if (AlreadyImported)
+return ToD;
+
ToD->setLexicalDeclContext(LexicalDC);
- Importer.Imported(D, ToD);
LexicalDC->addDeclInternal(ToD);
return ToD;
}
Decl *ASTNodeImporter::VisitTranslationUnitDecl(TranslationUnitDecl *D) {
TranslationUnitDecl *ToD =
Importer.getToContext().getTranslationUnitDecl();
-
[PATCH] D47632: [ASTImporter] Refactor Decl creation
martong created this revision.
martong added reviewers: a.sidorin, balazske, xazax.hun, r.stahl.
Herald added subscribers: cfe-commits, dkrupp, rnkovacs.
Generalize the creation of Decl nodes during Import. With this patch we do the
same things after and before a new AST node is created (::Create) The import
logic should be really simple, we create the node, then we mark that as
imported, then we recursively import the parts for that node and then set them
on that node. However, the AST is actually a graph, so we have to handle
circles. If we mark something as imported (`MapImported()`) then we return with
the corresponding `To` decl whenever we want to import that node again, this way
circles are handled. In order to make this algorithm work we must ensure
things, which are handled in the generic CreateDecl<> template:
- There are no `Import()` calls in between any node creation (::Create)
and the `MapImported()` call.
- Before actually creating an AST node (::Create), we must check if
the Node had been imported already, if yes then return with that one.
One very important case for this is connected to templates: we may
start an import both from the templated decl of a template and from
the template itself.
Now, the virtual `Imported` function is called in `ASTImporter::Impor(Decl *)`,
but only once, when the `Decl` is imported. One point of this refactor is to
separate responsibilities. The original `Imported()` had 3 responsibilities:
- notify subclasses when an import happened
- register the decl into `ImportedDecls`
- initialise the Decl (set attributes, etc)
Now all of these are in separate functions:
- `Imported`
- `MapImported`
- `InitializeImportedDecl`
I tried to check all the clients, I executed tests for `ExternalASTMerger.cpp`
and some unittests for lldb.
Repository:
rC Clang
https://reviews.llvm.org/D47632
Files:
include/clang/AST/ASTImporter.h
include/clang/AST/DeclBase.h
lib/AST/ASTImporter.cpp
lib/AST/ExternalASTMerger.cpp
Index: lib/AST/ExternalASTMerger.cpp
===
--- lib/AST/ExternalASTMerger.cpp
+++ lib/AST/ExternalASTMerger.cpp
@@ -154,7 +154,7 @@
ToContainer->setMustBuildLookupTable();
assert(Parent.CanComplete(ToContainer));
}
-return ASTImporter::Imported(From, To);
+return To;
}
ASTImporter () { return Reverse; }
};
@@ -229,7 +229,7 @@
SourceTag->getASTContext().getExternalSource()->CompleteType(SourceTag);
if (!SourceTag->getDefinition())
return false;
-Forward.Imported(SourceTag, Tag);
+Forward.MapImported(SourceTag, Tag);
Forward.ImportDefinition(SourceTag);
Tag->setCompleteDefinition(SourceTag->isCompleteDefinition());
return true;
@@ -248,7 +248,7 @@
SourceInterface);
if (!SourceInterface->getDefinition())
return false;
-Forward.Imported(SourceInterface, Interface);
+Forward.MapImported(SourceInterface, Interface);
Forward.ImportDefinition(SourceInterface);
return true;
});
@@ -304,7 +304,7 @@
void ExternalASTMerger::RecordOriginImpl(const DeclContext *ToDC, DCOrigin Origin,
ASTImporter ) {
Origins[ToDC] = Origin;
- Importer.ASTImporter::Imported(cast(Origin.DC), const_cast(cast(ToDC)));
+ Importer.ASTImporter::MapImported(cast(Origin.DC), const_cast(cast(ToDC)));
}
ExternalASTMerger::ExternalASTMerger(const ImporterTarget ,
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -76,6 +76,58 @@
public StmtVisitor {
ASTImporter
+// Wrapper for an overload set.
+template struct CallOverloadedCreateFun {
+ template
+ auto operator()(Args &&... args)
+ -> decltype(ToDeclT::Create(std::forward(args)...)) {
+return ToDeclT::Create(std::forward(args)...);
+ }
+};
+
+// Always use this function to create a Decl during import. There are
+// certain tasks which must be done after the Decl was created, e.g. we
+// must immediately register that as an imported Decl.
+// Returns a pair consisting of a pointer to the new or the already imported
+// Decl and a bool value set to true if the `FromD` had been imported
+// before.
+template
+std::pair CreateDecl(FromDeclT *FromD, Args &&... args) {
+ // There may be several overloads of ToDeclT::Create. We must make sure
+ // to call the one which would be chosen by the arguments, thus we use a
+ // wrapper for the overload set.
+ CallOverloadedCreateFun OC;
+ return CreateDecl(OC, FromD, std::forward(args)...);
+}
+// Use this overload directly only if a special create function must be
+// used, e.g. CXXRecordDecl::CreateLambda .
+template
+auto CreateDecl(CreateFunT CreateFun, FromDeclT *FromD,
