[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());
----------------
kpdev42 wrote:
> balazske wrote:
> > 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 *)`?
> The problem is that field attributes are required when we are adding a copy of a field to a structure in `VisitFieldDecl`. Attributes themselves are read in `CXXRecordDecl::addedMember` (see the call to `isZeroSize`). Adding them after `ImportImpl` is called is too late: record is already marked as non-empty. 
I understand now. But a comment could be added here to explain why the attribute import is needed here.


================
Comment at: clang/lib/AST/ASTImporter.cpp:8985
+Error ASTImporter::ImportAttrs(Decl *ToD, Decl *FromD) {
+  if (FromD->hasAttrs())
+    for (const Attr *FromAttr : FromD->getAttrs()) {
----------------
Should use braces in this case (https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145057