[PATCH] D145057: [clang][ASTImport] Add support for import of empty records

[Balázs Kéri via Phabricator via cfe-commits]

balazske added inline comments.


================
Comment at: clang/lib/AST/ASTImporter.cpp:3897
+  if (Err)
+    return std::move(Err);
   ToField->setAccess(D->getAccess());
----------------
I am not familiar with this use case, is there a path where the attributes are read from a `FieldDecl` before return from `VisitFieldDecl`? Probably `ImportImpl` is overridden? Importing the attributes here should work but not totally sure if it does not cause problems. Problematic case is if the attribute has pointer to a `Decl` or `Type` that is imported here in a state when the field is already created but not initialized. Another problem is that attributes are added a second time in `Import(Decl *)`. Can it work if the `ImportAttrs` is made a protected function and called from (overridden) `ImportImpl` (still there can be a second import in `Import(Decl *)`?


================
Comment at: clang/unittests/AST/ASTImporterTest.cpp:8157
+
+  CXXRecordDecl *ToD = cast<CXXRecordDecl>(Import(FromD, Lang_CXX20));
+  EXPECT_EQ(true, ToD->isEmpty());
----------------



================
Comment at: clang/unittests/AST/ASTImporterTest.cpp:8160
+  for (auto *FD : ToD->fields())
+    EXPECT_EQ(true, FD->hasAttr<NoUniqueAddressAttr>());
+}
----------------



================
Comment at: clang/unittests/AST/ASTImporterTest.cpp:8161
+    EXPECT_EQ(true, FD->hasAttr<NoUniqueAddressAttr>());
+}
+
----------------
Does this test fail without the changes applied? And does it not fail after (is the "Empty" value copied at import)?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145057/new/

https://reviews.llvm.org/D145057