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

[Emmett Neyman via Phabricator via cfe-commits]

emmettneyman marked 16 inline comments as done.
emmettneyman 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]>;
----------------
aaron.ballman wrote:
> Why does this have a `Clang` spelling in addition to the `GCC` spelling? I think it only needs the `GCC` spelling.
I kept the `Clang` spelling so that there's naming consistency between the two attributes (`requires_designator` and `requires_init`). I can remove it if it's redundant/unnecessary, let me know.


================
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
----------------
aaron.ballman wrote:
> Why is it valuable to differ from GCC's behavior in this regard?
The motivation behind GCC's attribute seems to be to avoid using positional arguments:
> The intent of this attribute is to allow the programmer to indicate that a structure’s layout may change, and that therefore relying on positional initialization will result in future breakage.

The motivation behind this attribute is a little bit different. Instead of avoiding the use of positional arguments, the goal of this attribute is to bring ObjC-style named parameters to C/C++ and to be able to enforce their use in certain situations. In line with this goal, we wanted to forbid the following pattern:
```
Foo foo;
foo.x = 42;
foo.y = 36;
```
That's why this attribute enforces that all declarations be brace-initialized.


================
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.
+  }];
----------------
aaron.ballman wrote:
> 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`?
The `requires_designator` attribute doesn't require that any of the fields have to be specified, only that designated initializer syntax has to be used. So, for the struct definition above, any of the following are valid:
```
Foo foo {};
Foo foo {.a = 1, .b = 2, .c = 3, .d = 4};
Foo foo {.b = 2, .c = 3};
```
Any of the fields can be left out and default-initialized. I've added this example to the lit tests for the attribute.


================
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<
----------------
aaron.ballman wrote:
> This attribute spelling seems wrong, it should be `designated_init`, no?
Does it make more sense to use `designated_init` or `requires_designator` as the primary spelling for this attribute? I chose to use the `requires_designator` spelling so that the two attributes have consistent and similar spellings. Let me know if you think it should be the other way around (or if you want the `Clang` spelling removed entirely).


================
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.
----------------
aaron.ballman wrote:
> Attribute spelling is stale in this comment.
See comments above about keeping this spelling.


================
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.
----------------
aaron.ballman wrote:
> 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.
See question above regarding attribute spelling.

I thought about this a lot because I agree, it feels wrong to remove the attribute once it is already added to a field, struct, etc. However I could not think of any other way or any other place to do this check. Since all fields of the struct need to have been processed already in order to check whether there are any non-public members, it seems like the only place to do this check is at the end of handling the entire `TagDecl`. When the struct-level and field-level attributes are attached, we don't yet have information about the access modifiers of all the members.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64380