aaron.ballman added inline comments.
================ Comment at: clang/include/clang/Basic/Attr.td:1948 +def RequiresDesignator : InheritableAttr { + let Spellings = [Clang<"requires_designator">, GCC<"designated_init">]; + let Subjects = SubjectList<[Record]>; ---------------- Why does this have a `Clang` spelling in addition to the `GCC` spelling? I think it only needs the `GCC` spelling. ================ Comment at: clang/include/clang/Basic/AttrDocs.td:1454-1455 +initializer syntax ([dcl.init.aggr]p3.1). +For a struct ``Foo`` with one integer field ``x``, the following declarations +are valid and invalid when this attribute is applied to the struct's definition. + ---------------- Please show the struct declaration itself as part of the code block. ================ Comment at: clang/include/clang/Basic/AttrDocs.td:1464-1466 +For a struct ``Foo`` with three integer fields ``x``, ``y``, and ``z``, the +following declarations are valid and invalid when this attribute is applied to +the struct's definition. ---------------- Same here. ================ Comment at: clang/include/clang/Basic/AttrDocs.td:1484-1485 +requires designated initializers be used rather than positional initializers. +This attribute additionally has a stronger restriction that declarations must be +brace-initialized (which is why ``Foo foo;`` is considered invalid above. The +final way this attribute differs from GCC's ``designated_init`` attribute is ---------------- Why is it valuable to differ from GCC's behavior in this regard? ================ Comment at: clang/include/clang/Basic/AttrDocs.td:1487 +final way this attribute differs from GCC's ``designated_init`` attribute is +that it can be applied to union and class types, as well as struct types. + }]; ---------------- Given that it can be added to class types, I wonder what the behavior should be for code like: ``` struct Foo { int a = 1; int b, c; int d = 4; }; Foo foo = {...}; ``` Does the initializer for `foo` have to specify `.a` and `.d`? ================ Comment at: clang/include/clang/Basic/AttrDocs.td:1498 +A field marked with this attribute may not be omitted or default-constructed. +For a struct ``Foo`` with a ``designated_init_required`` integer field ``x``, +the following declarations are valid and invalid. ---------------- Attribute name seems stale here. ================ Comment at: clang/include/clang/Basic/DiagnosticGroups.td:1069 + +// Warnings for requires_designator and requires_init attributes +def DesignatedInit : DiagGroup<"designated-init">; ---------------- Missing a full stop at the end of the comment. ================ Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:3537 +def note_declared_requires_designator_here : Note< + "required by 'requires_designator' attribute here">; +def warn_requires_init_failed : Warning< ---------------- This attribute spelling seems wrong, it should be `designated_init`, no? ================ Comment at: clang/lib/Sema/SemaDecl.cpp:11214 + if (const auto *TD = VDecl->getType().getTypePtr()->getAsTagDecl()) { + // If the type of the declaration is a struct/class and that type has the ---------------- You can drop the call to `getTypePtr()` and just use the overloaded `operator->()` instead. ================ Comment at: clang/lib/Sema/SemaDecl.cpp:11216 + // If the type of the declaration is a struct/class and that type has the + // require_designated_init attribute, check that the initializer has + // the proper designated initializer syntax. ---------------- Attribute spelling is stale in this comment. ================ Comment at: clang/lib/Sema/SemaDecl.cpp:15314 + // If this TagDecl has any non-public fields, all requires_designator and + // requires_init attributes should be ignored. ---------------- Attribute name is stale in this comment. This is the wrong place to do this work -- it should be done from SemaDeclAttr.cpp when processing the attribute. We should avoid adding the attribute in the first place rather than adding the attribute and then later removing it. ================ Comment at: clang/lib/Sema/SemaDecl.cpp:15318-15321 + for (auto const *FD : RD->fields()) { + if (FD->getAccess() != AS_public) + AllMembersPublic = false; + } ---------------- `HasNonPublicMember = llvm::any_of(RD->fields(), [](const FieldDecl *FD) { return FD->getAccess() != AS_public; });` ================ Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5313 +static void handleRequiresInitAttr(Sema &S, Decl *D, const ParsedAttr &AL) { + if (auto *FD = dyn_cast<FieldDecl>(D)) { + auto const *RD = FD->getParent(); ---------------- No need for this, you can just use `cast<>` directly, as the subject was already checked by the common attribute handling code. I would probably rewrite this as: ``` if (cast<FieldDecl>(D)->getParent()->isUnion()) { S.Diag(...); return; } D->addAttr(...); ``` ================ Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5314 + if (auto *FD = dyn_cast<FieldDecl>(D)) { + auto const *RD = FD->getParent(); + if (RD->isUnion()) ---------------- Do not use `auto` here as the type is not spelled out in the initialization. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64380/new/ https://reviews.llvm.org/D64380 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits