[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]>;
----------------
emmettneyman wrote:
> 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.
This is tricky. I don't see any value in `[[clang::requires_designator]]` in C++ mode because `[[gnu::designated_init]]` suffices, so I'd kind of prefer to see the `Clang` spelling go away. This attribute can also be used in C with `__attribute__((designated_init))` which is great, but I'd also like to see it be made available in C with double square bracket syntax (supported in C2x or via a feature flag). However, there is no `[[gnu::designated_init]]` supported by GCC, so it would be an abuse of the `gnu` namespace to add it there.  Having a `Clang` spelling gets around that issue. That said, I suspect GCC will support C2x attributes at some point, so I would expect this attribute to be supported there eventually, so the issue remains.

I think I've convinced myself that we should just drop the `Clang` spelling and wait to handle the C2x square bracket attribute until later, since C users can still use the `GNU` spelling. You should also rename the attribute `DesignatedInit` and change the other places with the older spelling.


================
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
----------------
emmettneyman wrote:
> 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.
I may still be missing something, but your rationale feels a bit more like a style-based choice than preventing common mistakes that are hard to track down; I find the GCC rationale to be more compelling. What do you think about matching the GCC behavior in the compiler warning, but adding the additional restrictions you'd like to see as a clang-tidy check?


================
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.
+  }];
----------------
emmettneyman wrote:
> 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.
So if I had a structure declaration like:
```
struct S {
  int a = 1;
  int b = 2;
};

S s;
```
This would fail because `s` is not initialized with curly braces? I don't think that's a particularly good behavior for C++.


================
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<
----------------
emmettneyman wrote:
> 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).
This is a bit moot since I think we'll want to drop the secondary spelling, but a better approach would be to use `%0` and pass in the `Attr` object directly so that we get the name used by the user instead of a static attribute name.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64380