a_sidorin added a comment.
Hi Gabor,
While importing methods looks harmless, importing fields can be a breaking
change. Do you have any test results on this?
================
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
----------------
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?
================
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);
----------------
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.
================
Comment at: lib/AST/ASTImporter.cpp:4551
+ for (auto *FromField : D->fields())
+ Importer.Import(FromField);
+
----------------
The result of import is unchecked here and below. Is it intentional?
================
Comment at: unittests/AST/ASTImporterTest.cpp:2722
+ ASSERT_FALSE(ToFun->hasBody());
+ auto *ImportedSpec = Import(FromSpec, Lang_CXX11);
+ ASSERT_TRUE(ImportedSpec);
----------------
3-space indentation.
================
Comment at: unittests/AST/ASTImporterTest.cpp:2763
+ ASSERT_FALSE(ToCtor->hasBody());
+ auto *ImportedSpec = Import(FromSpec, Lang_CXX11);
+ ASSERT_TRUE(ImportedSpec);
----------------
Broken indent.
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
