martong marked an inline comment as done. martong added inline comments.
================ Comment at: lib/AST/ASTImporter.cpp:4550 + // in the "From" context, but not in the "To" context. + for (auto *FromField : D->fields()) + Importer.Import(FromField); ---------------- a_sidorin wrote: > martong wrote: > > martong wrote: > > > a_sidorin wrote: > > > > Importing additional fields can change the layout of the > > > > specialization. For CSA, this usually results in strange assertions > > > > hard to debug. Could you please share the results of testing of this > > > > change? > > > > This change also doesn't seem to have related tests in this patch. > > > TLDR; We will not create additional fields. > > > > > > By the time when we import the field, we already know that the existing > > > specialization is structurally equivalent with the new one. > > > Since a ClassTemplateSpecializationDecl is the descendant of RecordDecl, > > > the structural equivalence check ensures that they have the exact same > > > fields. > > > When we import the field of the new spec and if there is an existing > > > FieldDecl in the "To" context, then no new FieldDecl will be created > > > (this is handled in `VisitFieldDecl` by first doing a lookup of existing > > > field with the same name and type). > > > This patch extends `VisitFieldDecl` in a way that we add new initializer > > > expressions to the existing FieldDecl, if it didn't have and in the > > > "From" context it has. > > > > > > For the record, I added a new test to make sure that a new FieldDecl > > > will not be created during the merge. > > This is the new test: > > `ODRViolationOfClassTemplateSpecializationsShouldBeReported`. It checks > > that it is not possible to add new fields to a specialization, rather an > > ODR violation is diagnosed. > Thank you for the explanation. However, I find the comment very misleading. > It tells: > ``` > // Check and merge those fields which have been instantiated > // in the "From" context, but not in the "To" context. > ``` > Would it be correct to change it to "Import field initializers that are still > not instantiated", or do I still misunderstand something? Yes, I agree that this comment is not precise and could be misleading, thank you for pointing this out. I changed this comment and others too. Repository: rC Clang https://reviews.llvm.org/D50451 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
