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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
