[PATCH] D50451: [ASTImporter] Fix import of class templates partial specialization

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin 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);
----------------
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?


Repository:
  rC Clang

https://reviews.llvm.org/D50451