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

[Aleksei Sidorin via Phabricator via cfe-commits]

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