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

[Gabor Marton via Phabricator via cfe-commits]

martong marked 5 inline comments as done.
martong added inline comments.


================
Comment at: lib/AST/ASTImporter.cpp:2872
         Importer.MapImported(D, FoundField);
+        // In case of a FieldDecl of a ClassTemplateSpecializationDecl, the
+        // initializer of a FieldDecl might not had been instantiated in the
----------------
a_sidorin wrote:
> Honestly speaking, I wonder about this behaviour because it doesn't look similar to instantiation of only methods that are used. Is it a common rule?
Yes, this is a common rule. The instantiation of an initializer is similar to the instantiation of default arguments in a sense that both are instantated only if they are being used.

To be more precise, quoting from Vandevoorde - C++ Templates The Complete Guide / 14.2.2 Instantiated Components:

... Default	function	call	arguments	are	considered	separately	when	instantiating
templates.	Specifically,	they	are	not	instantiated	unless	there	is	a	call	to	that
function	(or	member	function)	that	actually	makes	use	of	the	default	argument.
If,	on	the	other	hand,	the	function	is	called	with	explicit	arguments	that	override
the	default,	then	the	default	arguments	are	not	instantiated.
Similarly,	exception	specifications	and	**default	member	initializers**	are	not
instantiated	unless	they	are	needed.




================
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:
> 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.


================
Comment at: lib/AST/ASTImporter.cpp:4551
+      for (auto *FromField : D->fields())
+        Importer.Import(FromField);
+
----------------
a_sidorin wrote:
> The result of import is unchecked here and below. Is it intentional?
Yes, that is intentional. We plan to refactor all ASTImporter functions to provide a proper error handling mechanism, and in that change we would like to enforce the check of all import functions.
Unfortunately, currently we already have many places where we do not check the return value of import.


Repository:
  rC Clang

https://reviews.llvm.org/D50451