[clang][ASTImporter] Fix import of template parameter default values. #100100
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Balázs Kéri (balazske) ChangesDefault values of template parameters (non-type, type, template) were not correctly handled in the "inherited" case. This occurs if the first declaration contains the default value but a next one not. The default value is "inherited" from the first. In ASTImporter it was only possible to set the inherited status after the template object was created with the template parameters that were imported without handling the inherited case. The import function of the template parameter contains not enough information (previous declaration) to set the inherited-from status. After the template was created, default value of the parameters that should be inherited is reset to inherited mode. Full diff: https://github.com/llvm/llvm-project/pull/100100.diff 2 Files Affected:
diff --git a/clang/lib/AST/ASTImporter.cpp b/clang/lib/AST/ASTImporter.cpp
index 08ef09d353afc..1d9ea714780ce 100644
--- a/clang/lib/AST/ASTImporter.cpp
+++ b/clang/lib/AST/ASTImporter.cpp
@@ -359,6 +359,31 @@ namespace clang {
Params, Importer.getToContext().getTranslationUnitDecl());
}
+ template <typename TemplateParmDeclT>
+ void tryUpdateTemplateParmDeclInheritedFrom(NamedDecl *RecentParm,
+ NamedDecl *NewParm) {
+ if (auto *ParmT = dyn_cast<TemplateParmDeclT>(RecentParm)) {
+ if (ParmT->hasDefaultArgument()) {
+ auto *P = cast<TemplateParmDeclT>(NewParm);
+ P->removeDefaultArgument();
+ P->setInheritedDefaultArgument(Importer.ToContext, ParmT);
+ }
+ }
+ }
+
+ void updateTemplateParametersInheritedFrom(
+ const TemplateParameterList &RecentParams,
+ TemplateParameterList &NewParams) {
+ for (auto [Idx, Param] : enumerate(RecentParams)) {
+ tryUpdateTemplateParmDeclInheritedFrom<NonTypeTemplateParmDecl>(
+ Param, NewParams.getParam(Idx));
+ tryUpdateTemplateParmDeclInheritedFrom<TemplateTypeParmDecl>(
+ Param, NewParams.getParam(Idx));
+ tryUpdateTemplateParmDeclInheritedFrom<TemplateTemplateParmDecl>(
+ Param, NewParams.getParam(Idx));
+ }
+ }
+
public:
explicit ASTNodeImporter(ASTImporter &Importer) : Importer(Importer) {}
@@ -6138,6 +6163,9 @@ ExpectedDecl ASTNodeImporter::VisitClassTemplateDecl(ClassTemplateDecl *D) {
}
D2->setPreviousDecl(Recent);
+
+ updateTemplateParametersInheritedFrom(*(Recent->getTemplateParameters()),
+ **TemplateParamsOrErr);
}
return D2;
@@ -6452,6 +6480,9 @@ ExpectedDecl ASTNodeImporter::VisitVarTemplateDecl(VarTemplateDecl *D) {
ToTemplated->setPreviousDecl(PrevTemplated);
}
ToVarTD->setPreviousDecl(Recent);
+
+ updateTemplateParametersInheritedFrom(*(Recent->getTemplateParameters()),
+ **TemplateParamsOrErr);
}
return ToVarTD;
@@ -6724,6 +6755,9 @@ ASTNodeImporter::VisitFunctionTemplateDecl(FunctionTemplateDecl *D) {
TemplatedFD->setPreviousDecl(PrevTemplated);
}
ToFunc->setPreviousDecl(Recent);
+
+ updateTemplateParametersInheritedFrom(*(Recent->getTemplateParameters()),
+ *Params);
}
return ToFunc;
diff --git a/clang/unittests/AST/ASTImporterTest.cpp b/clang/unittests/AST/ASTImporterTest.cpp
index 6d987cc7e9ec6..5f6d9fec7052b 100644
--- a/clang/unittests/AST/ASTImporterTest.cpp
+++ b/clang/unittests/AST/ASTImporterTest.cpp
@@ -9681,59 +9681,40 @@ AST_MATCHER_P(EnumDecl, hasEnumConstName, StringRef, ConstName) {
return false;
}
-TEST_P(ASTImporterOptionSpecificTestBase, ImportAnonymousEnums) {
- const char *Code =
+TEST_P(ASTImporterOptionSpecificTestBase, ImportExistingAnonymousEnum) {
+ const char *ToCode =
R"(
struct A {
- enum { E1, E2 } x;
- enum { E3, E4 } y;
+ enum { E1, E2} x;
+ enum { E3, E4} y;
};
)";
- Decl *FromTU = getTuDecl(Code, Lang_CXX11);
- auto *FromEnumE1 = FirstDeclMatcher<EnumDecl>().match(
- FromTU, enumDecl(hasEnumConstName("E1")));
- auto *ImportedEnumE1 = Import(FromEnumE1, Lang_CXX11);
- EXPECT_TRUE(ImportedEnumE1);
- auto *FromEnumE3 = FirstDeclMatcher<EnumDecl>().match(
- FromTU, enumDecl(hasEnumConstName("E3")));
- auto *ImportedEnumE3 = Import(FromEnumE3, Lang_CXX11);
- EXPECT_TRUE(ImportedEnumE3);
- EXPECT_NE(ImportedEnumE1, ImportedEnumE3);
-}
-
-TEST_P(ASTImporterOptionSpecificTestBase, ImportFreeStandingAnonymousEnums) {
+ Decl *ToTU = getToTuDecl(ToCode, Lang_CXX11);
+ auto *ToE1 = FirstDeclMatcher<EnumDecl>().match(
+ ToTU, enumDecl(hasEnumConstName("E1")));
+ auto *ToE3 = FirstDeclMatcher<EnumDecl>().match(
+ ToTU, enumDecl(hasEnumConstName("E3")));
const char *Code =
R"(
struct A {
- enum { E1, E2 };
- enum { E3, E4 };
+ enum { E1, E2} x;
+ enum { E3, E4} y;
};
)";
Decl *FromTU = getTuDecl(Code, Lang_CXX11);
- auto *FromEnumE1 = FirstDeclMatcher<EnumDecl>().match(
+ auto *FromE1 = FirstDeclMatcher<EnumDecl>().match(
FromTU, enumDecl(hasEnumConstName("E1")));
- auto *ImportedEnumE1 = Import(FromEnumE1, Lang_CXX11);
- EXPECT_TRUE(ImportedEnumE1);
- auto *FromEnumE3 = FirstDeclMatcher<EnumDecl>().match(
+ auto *ImportedE1 = Import(FromE1, Lang_CXX11);
+ ASSERT_TRUE(ImportedE1);
+ EXPECT_EQ(ImportedE1, ToE1);
+ auto *FromE3 = FirstDeclMatcher<EnumDecl>().match(
FromTU, enumDecl(hasEnumConstName("E3")));
- auto *ImportedEnumE3 = Import(FromEnumE3, Lang_CXX11);
- EXPECT_TRUE(ImportedEnumE3);
- EXPECT_NE(ImportedEnumE1, ImportedEnumE3);
+ auto *ImportedE3 = Import(FromE3, Lang_CXX11);
+ ASSERT_TRUE(ImportedE3);
+ EXPECT_EQ(ImportedE3, ToE3);
}
-TEST_P(ASTImporterOptionSpecificTestBase, ImportExistingAnonymousEnums) {
- const char *ToCode =
- R"(
- struct A {
- enum { E1, E2 } x;
- enum { E3, E4 } y;
- };
- )";
- Decl *ToTU = getToTuDecl(ToCode, Lang_CXX11);
- auto *ToEnumE1 = FirstDeclMatcher<EnumDecl>().match(
- ToTU, enumDecl(hasEnumConstName("E1")));
- auto *ToEnumE3 = FirstDeclMatcher<EnumDecl>().match(
- ToTU, enumDecl(hasEnumConstName("E3")));
+TEST_P(ASTImporterOptionSpecificTestBase, ImportAnonymousEnum) {
const char *Code =
R"(
struct A {
@@ -9742,45 +9723,136 @@ TEST_P(ASTImporterOptionSpecificTestBase, ImportExistingAnonymousEnums) {
};
)";
Decl *FromTU = getTuDecl(Code, Lang_CXX11);
- auto *FromEnumE1 = FirstDeclMatcher<EnumDecl>().match(
+ auto *FromE1 = FirstDeclMatcher<EnumDecl>().match(
FromTU, enumDecl(hasEnumConstName("E1")));
- auto *ImportedEnumE1 = Import(FromEnumE1, Lang_CXX11);
- ASSERT_TRUE(ImportedEnumE1);
- EXPECT_EQ(ImportedEnumE1, ToEnumE1);
- auto *FromEnumE3 = FirstDeclMatcher<EnumDecl>().match(
+ auto *ImportedE1 = Import(FromE1, Lang_CXX11);
+ ASSERT_TRUE(ImportedE1);
+ auto *FromE3 = FirstDeclMatcher<EnumDecl>().match(
FromTU, enumDecl(hasEnumConstName("E3")));
- auto *ImportedEnumE3 = Import(FromEnumE3, Lang_CXX11);
- ASSERT_TRUE(ImportedEnumE3);
- EXPECT_EQ(ImportedEnumE3, ToEnumE3);
+ auto *ImportedE3 = Import(FromE3, Lang_CXX11);
+ ASSERT_TRUE(ImportedE3);
}
-TEST_P(ASTImporterOptionSpecificTestBase, ImportExistingEmptyAnonymousEnums) {
- const char *ToCode =
+struct ImportTemplateParmDeclDefaultValue
+ : public ASTImporterOptionSpecificTestBase {
+protected:
+ void checkTemplateParams(RedeclarableTemplateDecl *D) {
+ auto *CanD = cast<RedeclarableTemplateDecl>(D->getCanonicalDecl());
+ auto *CanNonTypeP = cast<NonTypeTemplateParmDecl>(
+ CanD->getTemplateParameters()->getParam(0));
+ auto *CanTypeP =
+ cast<TemplateTypeParmDecl>(CanD->getTemplateParameters()->getParam(1));
+ auto *CanTemplateP = cast<TemplateTemplateParmDecl>(
+ CanD->getTemplateParameters()->getParam(2));
+ EXPECT_FALSE(CanNonTypeP->getDefaultArgStorage().isInherited());
+ EXPECT_FALSE(CanTypeP->getDefaultArgStorage().isInherited());
+ EXPECT_FALSE(CanTemplateP->getDefaultArgStorage().isInherited());
+ for (Decl *Redecl : D->redecls()) {
+ auto *ReD = cast<RedeclarableTemplateDecl>(Redecl);
+ if (ReD != CanD) {
+ auto *NonTypeP = cast<NonTypeTemplateParmDecl>(
+ ReD->getTemplateParameters()->getParam(0));
+ auto *TypeP = cast<TemplateTypeParmDecl>(
+ ReD->getTemplateParameters()->getParam(1));
+ auto *TemplateP = cast<TemplateTemplateParmDecl>(
+ ReD->getTemplateParameters()->getParam(2));
+ EXPECT_TRUE(NonTypeP->getDefaultArgStorage().isInherited());
+ EXPECT_TRUE(TypeP->getDefaultArgStorage().isInherited());
+ EXPECT_TRUE(TemplateP->getDefaultArgStorage().isInherited());
+ EXPECT_EQ(NonTypeP->getDefaultArgStorage().getInheritedFrom(),
+ CanNonTypeP);
+ EXPECT_EQ(TypeP->getDefaultArgStorage().getInheritedFrom(), CanTypeP);
+ EXPECT_EQ(TemplateP->getDefaultArgStorage().getInheritedFrom(),
+ CanTemplateP);
+ }
+ }
+ }
+
+ void testImport(RedeclarableTemplateDecl *FromD) {
+ RedeclarableTemplateDecl *ToD = Import(FromD, Lang_CXX14);
+ checkTemplateParams(ToD);
+ }
+
+ const char *CodeFunction =
R"(
- struct A {
- enum {};
- };
+ template <class> struct X;
+
+ template <int A = 2, typename B = int, template<class> class C = X>
+ void f();
+ template <int A, typename B, template<class> class C>
+ void f();
+ template <int A, typename B, template<class> class C>
+ void f() {}
)";
- Decl *ToTU = getToTuDecl(ToCode, Lang_CXX11);
- auto *ToE1 = FirstDeclMatcher<EnumDecl>().match(ToTU, enumDecl());
- const char *Code =
+
+ const char *CodeClass =
R"(
- struct A {
- enum {};
- enum {};
- };
+ template <class> struct X;
+
+ template <int A = 2, typename B = int, template<class> class C = X>
+ struct S;
+ template <int A, typename B, template<class> class C>
+ struct S;
+ template <int A, typename B, template<class> class C>
+ struct S {};
)";
- Decl *FromTU = getTuDecl(Code, Lang_CXX11);
- auto *FromE1 = FirstDeclMatcher<EnumDecl>().match(FromTU, enumDecl());
- auto *ImportedE1 = Import(FromE1, Lang_CXX11);
- ASSERT_TRUE(ImportedE1);
- EXPECT_EQ(ImportedE1, ToE1);
- auto *FromE2 = LastDeclMatcher<EnumDecl>().match(FromTU, enumDecl());
- ASSERT_NE(FromE1, FromE2);
- auto *ImportedE2 = Import(FromE2, Lang_CXX11);
- ASSERT_TRUE(ImportedE2);
- // FIXME: These should not be equal, or the import should fail.
- EXPECT_EQ(ImportedE2, ToE1);
+
+ const char *CodeVar =
+ R"(
+ template <class> struct X;
+
+ template <int A = 2, typename B = int, template<class> class C = X>
+ extern int V;
+ template <int A, typename B, template<class> class C>
+ extern int V;
+ template <int A, typename B, template<class> class C>
+ int V = A;
+ )";
+};
+
+TEST_P(ImportTemplateParmDeclDefaultValue, ImportFunctionTemplate) {
+ Decl *FromTU = getTuDecl(CodeFunction, Lang_CXX14);
+ auto *FromLastD = LastDeclMatcher<FunctionTemplateDecl>().match(
+ FromTU, functionTemplateDecl(hasName("f")));
+ testImport(FromLastD);
+}
+
+TEST_P(ImportTemplateParmDeclDefaultValue, ImportExistingFunctionTemplate) {
+ getToTuDecl(CodeFunction, Lang_CXX14);
+ Decl *FromTU = getTuDecl(CodeFunction, Lang_CXX14);
+ auto *FromLastD = LastDeclMatcher<FunctionTemplateDecl>().match(
+ FromTU, functionTemplateDecl(hasName("f")));
+ testImport(FromLastD);
+}
+
+TEST_P(ImportTemplateParmDeclDefaultValue, ImportClassTemplate) {
+ Decl *FromTU = getTuDecl(CodeClass, Lang_CXX14);
+ auto *FromLastD = LastDeclMatcher<ClassTemplateDecl>().match(
+ FromTU, classTemplateDecl(hasName("S")));
+ testImport(FromLastD);
+}
+
+TEST_P(ImportTemplateParmDeclDefaultValue, ImportExistingClassTemplate) {
+ getToTuDecl(CodeClass, Lang_CXX14);
+ Decl *FromTU = getTuDecl(CodeClass, Lang_CXX14);
+ auto *FromLastD = LastDeclMatcher<ClassTemplateDecl>().match(
+ FromTU, classTemplateDecl(hasName("S")));
+ testImport(FromLastD);
+}
+
+TEST_P(ImportTemplateParmDeclDefaultValue, ImportVarTemplate) {
+ Decl *FromTU = getTuDecl(CodeVar, Lang_CXX14);
+ auto *FromLastD = LastDeclMatcher<VarTemplateDecl>().match(
+ FromTU, varTemplateDecl(hasName("V")));
+ testImport(FromLastD);
+}
+
+TEST_P(ImportTemplateParmDeclDefaultValue, ImportExistingVarTemplate) {
+ getToTuDecl(CodeVar, Lang_CXX14);
+ Decl *FromTU = getTuDecl(CodeVar, Lang_CXX14);
+ auto *FromLastD = LastDeclMatcher<VarTemplateDecl>().match(
+ FromTU, varTemplateDecl(hasName("V")));
+ testImport(FromLastD);
}
INSTANTIATE_TEST_SUITE_P(ParameterizedTests, ASTImporterLookupTableTest,
@@ -9866,6 +9938,9 @@ INSTANTIATE_TEST_SUITE_P(ParameterizedTests, ImportInjectedClassNameType,
INSTANTIATE_TEST_SUITE_P(ParameterizedTests, ImportMatrixType,
DefaultTestValuesForRunOptions);
+INSTANTIATE_TEST_SUITE_P(ParameterizedTests, ImportTemplateParmDeclDefaultValue,
+ DefaultTestValuesForRunOptions);
+
// FIXME: Make ImportOpenCLPipe test work.
// INSTANTIATE_TEST_SUITE_P(ParameterizedTests, ImportOpenCLPipe,
// DefaultTestValuesForRunOptions);
|
Default values of template parameters (non-type, type, template) were not correctly handled in the "inherited" case. This occurs if the first declaration contains the default value but a next one not. The default value is "inherited" from the first. In ASTImporter it was only possible to set the inherited status after the template object was created with the template parameters that were imported without handling the inherited case. The import function of the template parameter contains not enough information (previous declaration) to set the inherited-from status. After the template was created, default value of the parameters that should be inherited is reset to inherited mode.
balazske
force-pushed
the
astimporter_templparmdecldefaultinherited
branch
from
8a5280d to
e4440b8
Compare
NagyDonat
approved these changes
Jul 23, 2024
| } | ||
| } | ||
|
|
||
| void updateTemplateParametersInheritedFrom( |
There was a problem hiding this comment.
Suggested change
| void updateTemplateParametersInheritedFrom( | |
| /// Update the parameter list `NewParams` of a template declaration | |
| /// by "inheriting" default argument values from `RecentParams`, | |
| /// which is the parameter list of an earlier declaration of the | |
| /// same template. (Note that "inheriting" default argument values | |
| /// this way is completely unrelated to the usual object-oriented | |
| /// "inheritance" relationship between classes.) | |
| /// | |
| /// Note that the standard specifies that the same parameter cannot | |
| /// be given default arguments twice in the same scope; but in our | |
| /// case the parameter list `NewParams` is freshly imported from a | |
| /// different TU, so it's possible that both `RecentParams` and | |
| /// `NewParams` specify a default argument for the same parameter. | |
| /// In this case, the code will use the default argument taken from | |
| /// `RecentParams`. | |
| /// FIXME: Report an error if the earlier and the imported template | |
| /// declarations specify different default arguments. | |
| void updateTemplateParametersInheritedFrom( |
I felt that it is important to explain that the "Inherited" within the name of this function is unrelated to the usual object-oriented "inheritance" between classes; and as I started to write this comment, I also ended up documenting the goals of this function. (I hope that I was correct -- please fix this comment block if I misunderstood something!)
There was a problem hiding this comment.
Comment was added but different because the case is not exactly as described here.
There was a problem hiding this comment.
Thanks for the clarifications!
NagyDonat
approved these changes
Jul 26, 2024
| } | ||
| } | ||
|
|
||
| void updateTemplateParametersInheritedFrom( |
There was a problem hiding this comment.
Thanks for the clarifications!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Default values of template parameters (non-type, type, template) were not correctly handled in the "inherited" case. This occurs if the first declaration contains the default value but a next one not. The default value is "inherited" from the first.
In ASTImporter it was only possible to set the inherited status after the template object was created with the template parameters that were imported without handling the inherited case. The import function of the template parameter contains not enough information (previous declaration) to set the inherited-from status. After the template was created, default value of the parameters that should be inherited is reset to inherited mode.