[PATCH] D64380: Add 'require_designated_init' and 'required' attribute to clang

[Aaron Ballman via Phabricator via cfe-commits]

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