[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?
> 

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

[Emmett Neyman via Phabricator via cfe-commits]

emmettneyman updated this revision to Diff 211988.
emmettneyman marked 6 inline comments as done.
emmettneyman added a comment.

Addressed comments and feedback


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64380

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/SemaCXX/attr-requires-designator.cpp
  clang/test/SemaCXX/attr-requires-init.cpp

Index: clang/test/SemaCXX/attr-requires-init.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/attr-requires-init.cpp
@@ -0,0 +1,86 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+[[clang::requires_init]] int x;// expected-warning{{'requires_init' attribute only applies to non-static data members}}
+[[clang::requires_init]] void fun(int x) { // expected-warning{{'requires_init' attribute only applies to non-static data members}}
+  return;
+}
+struct [[clang::requires_init]] Foo { // expected-warning{{'requires_init' attribute only applies to non-static data members}}
+  int x;
+};
+
+// Struct with one required field
+struct Bar {
+  [[clang::requires_init]] int y; // expected-note 0+ {{enforced by 'requires_init' attribute here}}
+};
+
+// The following are invalid ways of initializing instances of this struct.
+Bar b1;// expected-warning{{initializer for variable b1 must explicitly initialize field y}}
+Bar b2{};  // expected-warning{{initializer for variable b2 must explicitly initialize field y}}
+Bar b3{1}; // expected-warning{{initializer for variable b3 must explicitly initialize field y}}
+
+// The following are valid ways of initializing instances of this struct.
+Bar b6{.y = 1};
+
+// Struct with multiple required fields
+struct Baz {
+  [[clang::requires_init]] int x; // expected-note 0+ {{enforced by 'requires_init' attribute here}}
+  int y;
+  [[clang::requires_init]] int z; // expected-note 0+ {{enforced by 'requires_init' attribute here}}
+};
+
+// The following are invalid ways of initializing instances of this struct.
+Baz z1; // expected-warning{{initializer for variable z1 must explicitly initialize field x}} expected-warning{{initializer for variable z1 must explicitly initialize field z}}
+Baz z2{};   // expected-warning{{initializer for variable z2 must explicitly initialize field x}} expected-warning{{initializer for variable z2 must explicitly initialize field z}}
+Baz z3{1, 2};   // expected-warning{{initializer for variable z3 must explicitly initialize field x}} expected-warning{{initializer for variable z3 must explicitly initialize field z}}
+Baz z4{1, 2, 3};// expected-warning{{initializer for variable z4 must explicitly initialize field x}} expected-warning{{initializer for variable z4 must explicitly initialize field z}}
+Baz z5{.x = 1, 2};  // expected-warning{{initializer for variable z5 must explicitly initialize field z}}
+Baz z6{.x = 1, .y = 2}; // expected-warning{{initializer for variable z6 must explicitly initialize field z}}
+
+// The following are valid ways of initializing instances of this struct.
+Baz z7{.x = 1, .y = 2, .z = 3};
+Baz z8{.x = 1, .z = 3};
+Baz z9{.x = 1, 2, .z = 3};
+
+// The requires_init attribute can also be applied to public fields of classes.
+class Cla {
+public:
+  [[clang::requires_init]] int x; // expected-note 0+ {{enforced by 'requires_init' attribute here}}
+  int y;
+};
+
+// The following are invalid ways of initializing instances of this class.
+Cla c1;// expected-warning{{initializer for variable c1 must explicitly initialize field x}}
+Cla c2{};  // expected-warning{{initializer for variable c2 must explicitly initialize field x}}
+Cla c3{1}; // expected-warning{{initializer for variable c3 must explicitly initialize field x}}
+Cla c4{1, 2};  // expected-warning{{initializer for variable c4 must explicitly initialize field x}}
+Cla c5{1, .y = 2}; // expected-warning{{initializer for variable c5 must explicitly initialize field x}}
+
+// The following are valid ways of initializing instances of this class.
+Cla c6{.x = 1};
+Cla c7{.x = 1, .y = 2};
+Cla c8{.x = 1, 2};
+
+// This attribute cannot be applied to fields of a union.
+union Uni {
+  [[clang::requires_init]] int x; // expected-warning{{'requires_init' attribute ignored}}
+  int y;
+};
+
+// If any of the fields in the record are non-public all requires_init
+// attributes in the record are ignored.
+struct PriMems {
+  [[clang::requires_init]] int x; // expected-warning{{'requires_init' attribute ignored}}
+private:
+  int y;
+};
+PriMems pm1;
+PriMems pm2();
+
+class PriClass {
+  int x;
+public:
+  

[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 

[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 , Decl *D, const ParsedAttr ) {
+  if (auto *FD = dyn_cast(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(D)->getParent()->isUnion()) {
  S.Diag(...);
  return;
}

D->addAttr(...);
```



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5314
+  if (auto 

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

[Aditya Kumar via Phabricator via cfe-commits]

hiraditya added a comment.

Thanks for addressing the feedback @emmettneyman , LGTM, deferring to other 
reviewers for final call.


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

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

[Emmett Neyman via Phabricator via cfe-commits]

emmettneyman updated this revision to Diff 210649.
emmettneyman added a comment.

Created diagnostic group, added behavior and documentation for private fields, 
added documentation regarding GCC attribute


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64380

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/SemaCXX/attr-requires-designator.cpp
  clang/test/SemaCXX/attr-requires-init.cpp

Index: clang/test/SemaCXX/attr-requires-init.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/attr-requires-init.cpp
@@ -0,0 +1,86 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+[[clang::requires_init]] int x;// expected-warning{{'requires_init' attribute only applies to non-static data members}}
+[[clang::requires_init]] void fun(int x) { // expected-warning{{'requires_init' attribute only applies to non-static data members}}
+  return;
+}
+struct [[clang::requires_init]] Foo { // expected-warning{{'requires_init' attribute only applies to non-static data members}}
+  int x;
+};
+
+// Struct with one required field
+struct Bar {
+  [[clang::requires_init]] int y; // expected-note 0+ {{enforced by 'requires_init' attribute here}}
+};
+
+// The following are invalid ways of initializing instances of this struct.
+Bar b1;// expected-warning{{initializer for variable b1 must explicitly initialize field y}}
+Bar b2{};  // expected-warning{{initializer for variable b2 must explicitly initialize field y}}
+Bar b3{1}; // expected-warning{{initializer for variable b3 must explicitly initialize field y}}
+
+// The following are valid ways of initializing instances of this struct.
+Bar b6{.y = 1};
+
+// Struct with multiple required fields
+struct Baz {
+  [[clang::requires_init]] int x; // expected-note 0+ {{enforced by 'requires_init' attribute here}}
+  int y;
+  [[clang::requires_init]] int z; // expected-note 0+ {{enforced by 'requires_init' attribute here}}
+};
+
+// The following are invalid ways of initializing instances of this struct.
+Baz z1; // expected-warning{{initializer for variable z1 must explicitly initialize field x}} expected-warning{{initializer for variable z1 must explicitly initialize field z}}
+Baz z2{};   // expected-warning{{initializer for variable z2 must explicitly initialize field x}} expected-warning{{initializer for variable z2 must explicitly initialize field z}}
+Baz z3{1, 2};   // expected-warning{{initializer for variable z3 must explicitly initialize field x}} expected-warning{{initializer for variable z3 must explicitly initialize field z}}
+Baz z4{1, 2, 3};// expected-warning{{initializer for variable z4 must explicitly initialize field x}} expected-warning{{initializer for variable z4 must explicitly initialize field z}}
+Baz z5{.x = 1, 2};  // expected-warning{{initializer for variable z5 must explicitly initialize field z}}
+Baz z6{.x = 1, .y = 2}; // expected-warning{{initializer for variable z6 must explicitly initialize field z}}
+
+// The following are valid ways of initializing instances of this struct.
+Baz z7{.x = 1, .y = 2, .z = 3};
+Baz z8{.x = 1, .z = 3};
+Baz z9{.x = 1, 2, .z = 3};
+
+// The requires_init attribute can also be applied to public fields of classes.
+class Cla {
+public:
+  [[clang::requires_init]] int x; // expected-note 0+ {{enforced by 'requires_init' attribute here}}
+  int y;
+};
+
+// The following are invalid ways of initializing instances of this class.
+Cla c1;// expected-warning{{initializer for variable c1 must explicitly initialize field x}}
+Cla c2{};  // expected-warning{{initializer for variable c2 must explicitly initialize field x}}
+Cla c3{1}; // expected-warning{{initializer for variable c3 must explicitly initialize field x}}
+Cla c4{1, 2};  // expected-warning{{initializer for variable c4 must explicitly initialize field x}}
+Cla c5{1, .y = 2}; // expected-warning{{initializer for variable c5 must explicitly initialize field x}}
+
+// The following are valid ways of initializing instances of this class.
+Cla c6{.x = 1};
+Cla c7{.x = 1, .y = 2};
+Cla c8{.x = 1, 2};
+
+// This attribute cannot be applied to fields of a union.
+union Uni {
+  [[clang::requires_init]] int x; // expected-warning{{'requires_init' attribute ignored}}
+  int y;
+};
+
+// If any of the fields in the record are non-public all requires_init
+// attributes in the record are ignored.
+struct PriMems {
+  [[clang::requires_init]] int x; // expected-warning{{'requires_init' attribute ignored}}
+private:
+  int y;
+};
+PriMems pm1;
+PriMems pm2();
+
+class 

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

[Emmett Neyman via Phabricator via cfe-commits]

emmettneyman planned changes to this revision.
emmettneyman added a comment.

Changes planned:

- Move diagnostics into a diagnostic group.
- Add behavior and test cases for private members of structs/classes.
- Investigate behavior of GCC `designated_init` attribute


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

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

[Emmett Neyman via Phabricator via cfe-commits]

emmettneyman added inline comments.



Comment at: clang/include/clang/Basic/AttrDocs.td:1448
 
+def RequireDesignatedInitDocs : Documentation {
+  let Category = DocCatType;

aaron.ballman wrote:
> The behavior should be documented as to what happens when you apply these 
> attributes to unions.
For now, adding a `requires_init` attribute to a field of a union will result 
in a warning that the attribute is ignored. In the future, I might submit 
another patch in the future adding behavior for this attribute for fields of a 
union.


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

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

[Emmett Neyman via Phabricator via cfe-commits]

emmettneyman updated this revision to Diff 209362.
emmettneyman marked 2 inline comments as done.
emmettneyman added a comment.

Added documentation about using the attributes with unions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64380

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/SemaCXX/attr-requires-designator.cpp
  clang/test/SemaCXX/attr-requires-init.cpp

Index: clang/test/SemaCXX/attr-requires-init.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/attr-requires-init.cpp
@@ -0,0 +1,67 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+[[clang::requires_init]] int x;// expected-warning{{'requires_init' attribute only applies to non-static data members}}
+[[clang::requires_init]] void fun(int x) { // expected-warning{{'requires_init' attribute only applies to non-static data members}}
+  return;
+}
+struct [[clang::requires_init]] Foo { // expected-warning{{'requires_init' attribute only applies to non-static data members}}
+  int x;
+};
+
+// Struct with one required field
+struct Bar {
+  [[clang::requires_init]] int y; // expected-note 0+ {{enforced by 'requires_init' attribute here}}
+};
+
+// The following are invalid ways of initializing instances of this struct.
+Bar b1;// expected-warning{{initializer for variable b1 must explicitly initialize field y}}
+Bar b2{};  // expected-warning{{initializer for variable b2 must explicitly initialize field y}}
+Bar b3{1}; // expected-warning{{initializer for variable b3 must explicitly initialize field y}}
+
+// The following are valid ways of initializing instances of this struct.
+Bar b6{.y = 1};
+
+// Struct with multiple required fields
+struct Baz {
+  [[clang::requires_init]] int x; // expected-note 0+ {{enforced by 'requires_init' attribute here}}
+  int y;
+  [[clang::requires_init]] int z; // expected-note 0+ {{enforced by 'requires_init' attribute here}}
+};
+
+// The following are invalid ways of initializing instances of this struct.
+Baz z1; // expected-warning{{initializer for variable z1 must explicitly initialize field x}} expected-warning{{initializer for variable z1 must explicitly initialize field z}}
+Baz z2{};   // expected-warning{{initializer for variable z2 must explicitly initialize field x}} expected-warning{{initializer for variable z2 must explicitly initialize field z}}
+Baz z3{1, 2};   // expected-warning{{initializer for variable z3 must explicitly initialize field x}} expected-warning{{initializer for variable z3 must explicitly initialize field z}}
+Baz z4{1, 2, 3};// expected-warning{{initializer for variable z4 must explicitly initialize field x}} expected-warning{{initializer for variable z4 must explicitly initialize field z}}
+Baz z5{.x = 1, 2};  // expected-warning{{initializer for variable z5 must explicitly initialize field z}}
+Baz z6{.x = 1, .y = 2}; // expected-warning{{initializer for variable z6 must explicitly initialize field z}}
+
+// The following are valid ways of initializing instances of this struct.
+Baz z7{.x = 1, .y = 2, .z = 3};
+Baz z8{.x = 1, .z = 3};
+Baz z9{.x = 1, 2, .z = 3};
+
+// The required attribute can also be applied to public fields of classes.
+class Cla {
+public:
+  [[clang::requires_init]] int x; // expected-note 0+ {{enforced by 'requires_init' attribute here}}
+  int y;
+};
+
+// The following are invalid ways of initializing instances of this class.
+Cla c1;// expected-warning{{initializer for variable c1 must explicitly initialize field x}}
+Cla c2{};  // expected-warning{{initializer for variable c2 must explicitly initialize field x}}
+Cla c3{1}; // expected-warning{{initializer for variable c3 must explicitly initialize field x}}
+Cla c4{1, 2};  // expected-warning{{initializer for variable c4 must explicitly initialize field x}}
+Cla c5{1, .y = 2}; // expected-warning{{initializer for variable c5 must explicitly initialize field x}}
+
+// The following are valid ways of initializing instances of this class.
+Cla c6{.x = 1};
+Cla c7{.x = 1, .y = 2};
+Cla c8{.x = 1, 2};
+
+// This attribute cannot be applied to fields of a union.
+union Uni {
+  [[clang::requires_init]] int x; // expected-warning{{'requires_init' attribute ignored}}
+  int y;
+};
Index: clang/test/SemaCXX/attr-requires-designator.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/attr-requires-designator.cpp
@@ -0,0 +1,102 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+// The requires_designator attribute only applies to types. It will
+// generate a warning when attached to 

[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">];
+  let Subjects = SubjectList<[Record]>;

emmettneyman wrote:
> compnerd wrote:
> > aaron.ballman wrote:
> > > Hmm, after making this suggestion, I noticed that GCC seems to support a 
> > > similar attribute named `designated_init` 
> > > (https://gcc.gnu.org/onlinedocs/gcc/Common-Type-Attributes.html#Common-Type-Attributes).
> > >  Was your goal to support the same thing GCC supported?
> > `designated_init` is a suggestion, the original patch seemed to be a 
> > stronger version.  I think that we should be supporting the GNU spelling 
> > for `designated_init` and can support a Clang spelling of 
> > `requires_designated_init` if the goal is to have the stronger guarantee 
> > that this *must* happen.
> I hadn't known about the GCC attribute until now. Yes, the 
> `requires_designator` (originally `require_designated_init`) attribute wants 
> to enforce the same thing I believe. I couldn't find more documentation for 
> the GCC `designated_init` attribute so it's a little tough to tell whether 
> the behavior is the exact same. The attribute in this patch allows a field to 
> be default constructed (unless the other attribute is applied to that 
> specific field) but enforces that a brace initializer must be used. So `Foo 
> foo {};` would be valid (every field is default constructed) but `Foo foo;` 
> would not be valid. I'm not sure if that's the same behavior the GCC 
> attribute is trying to enforce. But on a high level, both are trying to 
> prohibit using positional args when declaring a struct.
> 
> @compnerd I don't mind this attribute generating warnings rather than errors. 
> It's ok for this attribute to be a "suggestion" as well. Like @aaron.ballman 
> mentioned, "users can always use -Werror to strengthen their own 
> requirements."
I think the correct approach then is: add this attribute under the name 
`designated_init` with the `GCC` spelling kind and match GCC's behavior for it 
(or, if we don't want to match GCC's behavior, note where we differ and why). 
We should put its diagnostics under a `-Wdesignated-init` warning group.

The next question is: are you interested in finding out whether GCC would be 
willing to implement the `requires_init` attribute? If GCC is interested in 
picking it up, then we can name it with the `GCC` spelling as well. If the GCC 
folks are not interested in picking up the attribute, then we should probably 
leave it with the `Clang` spelling. I'd recommend we put the diagnostics into 
the `-Wdesignated-init` warning group as well.

I think both of these attributes should also be available in C2x mode as well, 
but that can probably be done in a follow-up patch as it will involve tablegen 
changes and opens up other questions (i.e., does GCC handle C2x attributes yet, 
and if they do, are they making `designated_init` available in that mode?).


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

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

[Emmett Neyman via Phabricator via cfe-commits]

emmettneyman marked 3 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">];
+  let Subjects = SubjectList<[Record]>;

compnerd wrote:
> aaron.ballman wrote:
> > Hmm, after making this suggestion, I noticed that GCC seems to support a 
> > similar attribute named `designated_init` 
> > (https://gcc.gnu.org/onlinedocs/gcc/Common-Type-Attributes.html#Common-Type-Attributes).
> >  Was your goal to support the same thing GCC supported?
> `designated_init` is a suggestion, the original patch seemed to be a stronger 
> version.  I think that we should be supporting the GNU spelling for 
> `designated_init` and can support a Clang spelling of 
> `requires_designated_init` if the goal is to have the stronger guarantee that 
> this *must* happen.
I hadn't known about the GCC attribute until now. Yes, the 
`requires_designator` (originally `require_designated_init`) attribute wants to 
enforce the same thing I believe. I couldn't find more documentation for the 
GCC `designated_init` attribute so it's a little tough to tell whether the 
behavior is the exact same. The attribute in this patch allows a field to be 
default constructed (unless the other attribute is applied to that specific 
field) but enforces that a brace initializer must be used. So `Foo foo {};` 
would be valid (every field is default constructed) but `Foo foo;` would not be 
valid. I'm not sure if that's the same behavior the GCC attribute is trying to 
enforce. But on a high level, both are trying to prohibit using positional args 
when declaring a struct.

@compnerd I don't mind this attribute generating warnings rather than errors. 
It's ok for this attribute to be a "suggestion" as well. Like @aaron.ballman 
mentioned, "users can always use -Werror to strengthen their own requirements."


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

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

[Emmett Neyman via Phabricator via cfe-commits]

emmettneyman updated this revision to Diff 209256.
emmettneyman added a comment.

Small changes: adding `auto`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64380

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/SemaCXX/attr-requires-designator.cpp
  clang/test/SemaCXX/attr-requires-init.cpp

Index: clang/test/SemaCXX/attr-requires-init.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/attr-requires-init.cpp
@@ -0,0 +1,61 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+[[clang::requires_init]] int x;// expected-warning{{'requires_init' attribute only applies to non-static data members}}
+[[clang::requires_init]] void fun(int x) { // expected-warning{{'requires_init' attribute only applies to non-static data members}}
+  return;
+}
+struct [[clang::requires_init]] Foo { // expected-warning{{'requires_init' attribute only applies to non-static data members}}
+  int x;
+};
+
+// Struct with one required field
+struct Bar {
+  [[clang::requires_init]] int y; // expected-note 0+ {{enforced by 'requires_init' attribute here}}
+};
+
+// The following are invalid ways of initializing instances of this struct.
+Bar b1;// expected-warning{{initializer for variable b1 must explicitly initialize field y}}
+Bar b2{};  // expected-warning{{initializer for variable b2 must explicitly initialize field y}}
+Bar b3{1}; // expected-warning{{initializer for variable b3 must explicitly initialize field y}}
+
+// The following are valid ways of initializing instances of this struct.
+Bar b6{.y = 1};
+
+// Struct with multiple required fields
+struct Baz {
+  [[clang::requires_init]] int x; // expected-note 0+ {{enforced by 'requires_init' attribute here}}
+  int y;
+  [[clang::requires_init]] int z; // expected-note 0+ {{enforced by 'requires_init' attribute here}}
+};
+
+// The following are invalid ways of initializing instances of this struct.
+Baz z1; // expected-warning{{initializer for variable z1 must explicitly initialize field x}} expected-warning{{initializer for variable z1 must explicitly initialize field z}}
+Baz z2{};   // expected-warning{{initializer for variable z2 must explicitly initialize field x}} expected-warning{{initializer for variable z2 must explicitly initialize field z}}
+Baz z3{1, 2};   // expected-warning{{initializer for variable z3 must explicitly initialize field x}} expected-warning{{initializer for variable z3 must explicitly initialize field z}}
+Baz z4{1, 2, 3};// expected-warning{{initializer for variable z4 must explicitly initialize field x}} expected-warning{{initializer for variable z4 must explicitly initialize field z}}
+Baz z5{.x = 1, 2};  // expected-warning{{initializer for variable z5 must explicitly initialize field z}}
+Baz z6{.x = 1, .y = 2}; // expected-warning{{initializer for variable z6 must explicitly initialize field z}}
+
+// The following are valid ways of initializing instances of this struct.
+Baz z7{.x = 1, .y = 2, .z = 3};
+Baz z8{.x = 1, .z = 3};
+Baz z9{.x = 1, 2, .z = 3};
+
+// The required attribute can also be applied to public fields of classes.
+class Cla {
+public:
+  [[clang::requires_init]] int x; // expected-note 0+ {{enforced by 'requires_init' attribute here}}
+  int y;
+};
+
+// The following are invalid ways of initializing instances of this class.
+Cla c1;// expected-warning{{initializer for variable c1 must explicitly initialize field x}}
+Cla c2{};  // expected-warning{{initializer for variable c2 must explicitly initialize field x}}
+Cla c3{1}; // expected-warning{{initializer for variable c3 must explicitly initialize field x}}
+Cla c4{1, 2};  // expected-warning{{initializer for variable c4 must explicitly initialize field x}}
+Cla c5{1, .y = 2}; // expected-warning{{initializer for variable c5 must explicitly initialize field x}}
+
+// The following are valid ways of initializing instances of this class.
+Cla c6{.x = 1};
+Cla c7{.x = 1, .y = 2};
+Cla c8{.x = 1, 2};
Index: clang/test/SemaCXX/attr-requires-designator.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/attr-requires-designator.cpp
@@ -0,0 +1,102 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+// The requires_designator attribute only applies to types. It will
+// generate a warning when attached to variables, functions, arrays, etc.
+[[clang::requires_designator]] int x;// expected-warning{{'requires_designator' attribute only applies to structs, unions, and classes}}
+[[clang::requires_designator]] void fun(int x) { // 

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

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:1948
+def RequiresDesignator : InheritableAttr {
+  let Spellings = [Clang<"requires_designator">];
+  let Subjects = SubjectList<[Record]>;

aaron.ballman wrote:
> Hmm, after making this suggestion, I noticed that GCC seems to support a 
> similar attribute named `designated_init` 
> (https://gcc.gnu.org/onlinedocs/gcc/Common-Type-Attributes.html#Common-Type-Attributes).
>  Was your goal to support the same thing GCC supported?
`designated_init` is a suggestion, the original patch seemed to be a stronger 
version.  I think that we should be supporting the GNU spelling for 
`designated_init` and can support a Clang spelling of 
`requires_designated_init` if the goal is to have the stronger guarantee that 
this *must* happen.


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

[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">];
+  let Subjects = SubjectList<[Record]>;

Hmm, after making this suggestion, I noticed that GCC seems to support a 
similar attribute named `designated_init` 
(https://gcc.gnu.org/onlinedocs/gcc/Common-Type-Attributes.html#Common-Type-Attributes).
 Was your goal to support the same thing GCC supported?



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:3534
+def warn_requires_designator_failed : Warning<
+  "variable declaration does not use designated initializer syntax">;
+def note_declared_requires_designator_here : Note<

These new warnings should be controlled under a diagnostic group. I would 
recommend `designated-init` because that's what's used by GCC for their 
attribute, but that's only because I am assuming you want your attribute to 
match their behavior.



Comment at: clang/lib/Sema/SemaDecl.cpp:11220
+  if (const auto *ILE = dyn_cast(Init)) {
+for (auto *I : ILE->inits()) {
+  if (auto *DIE = dyn_cast(I))

`const auto *` here as well?



Comment at: clang/lib/Sema/SemaDecl.cpp:11221
+for (auto *I : ILE->inits()) {
+  if (auto *DIE = dyn_cast(I))
+continue;

You don't use `DIE` -- this should switch to an `isa<>` instead.



Comment at: clang/lib/Sema/SemaDecl.cpp:11254
+if (const auto *ILE = dyn_cast(Init)) {
+  for (auto *I : ILE->inits()) {
+if (auto *DIE = dyn_cast(I)) {

`const auto *` here and below?


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

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

[Emmett Neyman via Phabricator via cfe-commits]

emmettneyman planned changes to this revision.
emmettneyman added a comment.

Working on adding more information to the documentation and adding more 
in-depth unit tests.


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

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

[Emmett Neyman via Phabricator via cfe-commits]

emmettneyman added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:3533-3541
+def err_require_designated_init_failed : Error<
+  "variable declaration does not use designated initializer syntax">;
+def note_declared_required_designated_init_here : Note<
+  "required by 'require_designated_init' attribute here">;
+
+def err_required_failed : Error<
+  "initializer for variable %0 must explicitly initialize field %1">;

aaron.ballman wrote:
> I'm a little uncomfortable with these being errors rather than warnings. I 
> think of these attributes as being preferences rather than requirements; the 
> code isn't *wrong* if it fails to use the designated initializer (it's 
> conforming to C or C++, does not have UB, etc) and other implementations are 
> free to ignore those attributes and the end result will be identical to 
> what's produced by Clang.
> 
> What do you think about turning these into warnings? Users can always use 
> `-Werror` to strengthen their own requirements.
That makes sense, I'll make the change.


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

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

[Emmett Neyman via Phabricator via cfe-commits]

emmettneyman updated this revision to Diff 209097.
emmettneyman marked 32 inline comments as done.
emmettneyman added a comment.

Addressed several of the comments regarding naming and style.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64380

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/SemaCXX/attr-requires-designator.cpp
  clang/test/SemaCXX/attr-requires-init.cpp

Index: clang/test/SemaCXX/attr-requires-init.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/attr-requires-init.cpp
@@ -0,0 +1,61 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+[[clang::requires_init]] int x;// expected-warning{{'requires_init' attribute only applies to non-static data members}}
+[[clang::requires_init]] void fun(int x) { // expected-warning{{'requires_init' attribute only applies to non-static data members}}
+  return;
+}
+struct [[clang::requires_init]] Foo { // expected-warning{{'requires_init' attribute only applies to non-static data members}}
+  int x;
+};
+
+// Struct with one required field
+struct Bar {
+  [[clang::requires_init]] int y; // expected-note 0+ {{enforced by 'requires_init' attribute here}}
+};
+
+// The following are invalid ways of initializing instances of this struct.
+Bar b1;// expected-warning{{initializer for variable b1 must explicitly initialize field y}}
+Bar b2{};  // expected-warning{{initializer for variable b2 must explicitly initialize field y}}
+Bar b3{1}; // expected-warning{{initializer for variable b3 must explicitly initialize field y}}
+
+// The following are valid ways of initializing instances of this struct.
+Bar b6{.y = 1};
+
+// Struct with multiple required fields
+struct Baz {
+  [[clang::requires_init]] int x; // expected-note 0+ {{enforced by 'requires_init' attribute here}}
+  int y;
+  [[clang::requires_init]] int z; // expected-note 0+ {{enforced by 'requires_init' attribute here}}
+};
+
+// The following are invalid ways of initializing instances of this struct.
+Baz z1; // expected-warning{{initializer for variable z1 must explicitly initialize field x}} expected-warning{{initializer for variable z1 must explicitly initialize field z}}
+Baz z2{};   // expected-warning{{initializer for variable z2 must explicitly initialize field x}} expected-warning{{initializer for variable z2 must explicitly initialize field z}}
+Baz z3{1, 2};   // expected-warning{{initializer for variable z3 must explicitly initialize field x}} expected-warning{{initializer for variable z3 must explicitly initialize field z}}
+Baz z4{1, 2, 3};// expected-warning{{initializer for variable z4 must explicitly initialize field x}} expected-warning{{initializer for variable z4 must explicitly initialize field z}}
+Baz z5{.x = 1, 2};  // expected-warning{{initializer for variable z5 must explicitly initialize field z}}
+Baz z6{.x = 1, .y = 2}; // expected-warning{{initializer for variable z6 must explicitly initialize field z}}
+
+// The following are valid ways of initializing instances of this struct.
+Baz z7{.x = 1, .y = 2, .z = 3};
+Baz z8{.x = 1, .z = 3};
+Baz z9{.x = 1, 2, .z = 3};
+
+// The required attribute can also be applied to public fields of classes.
+class Cla {
+public:
+  [[clang::requires_init]] int x; // expected-note 0+ {{enforced by 'requires_init' attribute here}}
+  int y;
+};
+
+// The following are invalid ways of initializing instances of this class.
+Cla c1;// expected-warning{{initializer for variable c1 must explicitly initialize field x}}
+Cla c2{};  // expected-warning{{initializer for variable c2 must explicitly initialize field x}}
+Cla c3{1}; // expected-warning{{initializer for variable c3 must explicitly initialize field x}}
+Cla c4{1, 2};  // expected-warning{{initializer for variable c4 must explicitly initialize field x}}
+Cla c5{1, .y = 2}; // expected-warning{{initializer for variable c5 must explicitly initialize field x}}
+
+// The following are valid ways of initializing instances of this class.
+Cla c6{.x = 1};
+Cla c7{.x = 1, .y = 2};
+Cla c8{.x = 1, 2};
Index: clang/test/SemaCXX/attr-requires-designator.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/attr-requires-designator.cpp
@@ -0,0 +1,102 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+// The requires_designator attribute only applies to types. It will
+// generate a warning when attached to variables, functions, arrays, etc.
+[[clang::requires_designator]] int x;// expected-warning{{'requires_designator' attribute only applies to structs, unions, and 

[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:1947
 
+def RequireDesignatedInit : InheritableAttr {
+  let Spellings = [CXX11<"clang", "require_designated_init">];

This should be `RequiresDesignator`.



Comment at: clang/include/clang/Basic/Attr.td:1948
+def RequireDesignatedInit : InheritableAttr {
+  let Spellings = [CXX11<"clang", "require_designated_init">];
+  let Subjects = SubjectList<[Type]>;

I'd prefer this be named `requires_designator`.



Comment at: clang/include/clang/Basic/Attr.td:1949
+  let Spellings = [CXX11<"clang", "require_designated_init">];
+  let Subjects = SubjectList<[Type]>;
+  let Documentation = [RequireDesignatedInitDocs];

I'm a bit confused by this subject -- it's a declaration attribute that 
appertains to types? Why is this not `RecordDecl` instead?



Comment at: clang/include/clang/Basic/Attr.td:1953
+
+def Required : InheritableAttr {
+  let Spellings = [CXX11<"clang", "designated_init_required">];

This should be `RequiresInit`



Comment at: clang/include/clang/Basic/Attr.td:1954
+def Required : InheritableAttr {
+  let Spellings = [CXX11<"clang", "designated_init_required">];
+  let Subjects = SubjectList<[Field]>;

This should also use the `Clang` spelling. Also, I'd prefer this be named 
`requires_init`.

As it stands, these two attribute names are *way* too similar.



Comment at: clang/include/clang/Basic/Attr.td:1948
+def RequireDesignatedInit : InheritableAttr {
+  let Spellings = [GNU<"require_designated_init">];
+  let Subjects = SubjectList<[Type]>;

compnerd wrote:
> This seems like an extension?  I think that we want to explicitly mark this 
> as an extension, as this is not available in GCC AFAIK.
This should use the `Clang` attribute spelling rather than `CXX11`.



Comment at: clang/include/clang/Basic/AttrDocs.td:1448
 
+def RequireDesignatedInitDocs : Documentation {
+  let Category = DocCatType;

The behavior should be documented as to what happens when you apply these 
attributes to unions.



Comment at: clang/include/clang/Basic/AttrDocs.td:1451
+  let Content = [{
+This attribute can be applied to a struct definition to require that anytime a
+variable of that struct's type is declared, the declaration uses designated

anytime -> any time



Comment at: clang/include/clang/Basic/AttrDocs.td:1452
+This attribute can be applied to a struct definition to require that anytime a
+variable of that struct's type is declared, the declaration uses designated
+initializer syntax ([dcl.init.aggr]p3.1).

struct's -> structure's



Comment at: clang/include/clang/Basic/AttrDocs.td:1484
+This attribute can be applied to a field definition within a struct or a class
+to require that anytime a variable of the struct/class's type is declared, that
+field must be initialized using designated initializer syntax 
([dcl.init.aggr]p3.1).

anytime -> any time
struct/class -> struct



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:3533-3541
+def err_require_designated_init_failed : Error<
+  "variable declaration does not use designated initializer syntax">;
+def note_declared_required_designated_init_here : Note<
+  "required by 'require_designated_init' attribute here">;
+
+def err_required_failed : Error<
+  "initializer for variable %0 must explicitly initialize field %1">;

I'm a little uncomfortable with these being errors rather than warnings. I 
think of these attributes as being preferences rather than requirements; the 
code isn't *wrong* if it fails to use the designated initializer (it's 
conforming to C or C++, does not have UB, etc) and other implementations are 
free to ignore those attributes and the end result will be identical to what's 
produced by Clang.

What do you think about turning these into warnings? Users can always use 
`-Werror` to strengthen their own requirements.



Comment at: clang/lib/Sema/SemaDecl.cpp:11214
 
+  if (auto *TD = VDecl->getType().getTypePtr()->getAsTagDecl()) {
+// If the type of the declaration is a struct/class and that type has the

Any reason this shouldn't be `const auto *`? (Propagated elsewhere.)



Comment at: clang/lib/Sema/SemaDecl.cpp:11220-11221
+  if (auto *ILE = dyn_cast(Init)) {
+for (unsigned i = 0; i < ILE->getNumInits(); i++) {
+  Expr *init = ILE->getInit(i);
+  if (auto *DIE = dyn_cast(init))

You can use a range-based for loop over `inits()`, I believe. Also, the 
variable should be named `Init` to meet the coding style.



Comment at: clang/lib/Sema/SemaDecl.cpp:11228
+  << 

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

[Emmett Neyman via Phabricator via cfe-commits]

emmettneyman marked 2 inline comments as done.
emmettneyman added inline comments.



Comment at: clang/test/SemaCXX/attr-designated-init-required.cpp:3
+
+#define ATTR [[clang::designated_init_required]]
+

compnerd wrote:
> Why the macro?
I modeled this file after 
`test/SemaCXX/attr-require-constant-initialization.cpp` where a macro is 
defined at the top. I assume just to keep things neat.



Comment at: clang/test/SemaCXX/attr-require-designated-init.cpp:3
+
+#define ATTR [[clang::require_designated_init]]
+

compnerd wrote:
> Why the macro?
See above.


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

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

[Emmett Neyman via Phabricator via cfe-commits]

emmettneyman added a comment.

In D64380#1577350 , @compnerd wrote:

> I don't see any cases where `[[clang::required]]` is tested, am I missing 
> something?


I renamed the attribute at your suggestion. It's now called 
`[[clang::designated_init_required]`.


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

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

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added a comment.

I don't see any cases where `[[clang::required]]` is tested, am I missing 
something?




Comment at: clang/test/SemaCXX/attr-designated-init-required.cpp:3
+
+#define ATTR [[clang::designated_init_required]]
+

Why the macro?



Comment at: clang/test/SemaCXX/attr-require-designated-init.cpp:3
+
+#define ATTR [[clang::require_designated_init]]
+

Why the macro?


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

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

[Emmett Neyman via Phabricator via cfe-commits]

emmettneyman updated this revision to Diff 208842.
emmettneyman added a comment.

slight change to specification reference format


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64380

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/SemaCXX/attr-designated-init-required.cpp
  clang/test/SemaCXX/attr-require-designated-init.cpp

Index: clang/test/SemaCXX/attr-require-designated-init.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/attr-require-designated-init.cpp
@@ -0,0 +1,104 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+#define ATTR [[clang::require_designated_init]]
+
+// The require_designated_init attribute only applies to types. It will
+// generate a warning when attached to variables, functions, arrays, etc.
+int ATTR x;// expected-error{{'require_designated_init' attribute cannot be applied to types}}
+void ATTR fun(int x) { // expected-error{{'require_designated_init' attribute cannot be applied to types}}
+  return;
+}
+int ATTR arr[10]; // expected-error{{'require_designated_init' attribute cannot be applied to types}}
+
+// Struct with one field with require_designated_init attribute
+struct ATTR Foo { // expected-note 0+ {{required by 'require_designated_init' attribute here}}
+  int a;
+};
+
+// The following are invalid ways of initializing instances of this struct.
+Foo f1;   // expected-error{{variable declaration does not use designated initializer syntax}}
+Foo f2{1};// expected-error{{variable declaration does not use designated initializer syntax}}
+Foo f3 = {1}; // expected-error{{variable declaration does not use designated initializer syntax}}
+// The following are valid ways of initializing instances of this struct.
+Foo f4{};
+Foo f5 = {};
+Foo f6{.a = 1};
+Foo f7 = {.a = 1};
+
+// Struct with multiple fields wth require_designated_init attribute
+struct ATTR Bar { // expected-note 0+ {{required by 'require_designated_init' attribute here}}
+  int b;
+  int c;
+};
+
+// The following are invalid ways of initializing instances of this struct.
+Bar b1;   // expected-error{{variable declaration does not use designated initializer syntax}}
+Bar b2{1, 2}; // expected-error{{variable declaration does not use designated initializer syntax}}
+Bar b3 = {1, 2};  // expected-error{{variable declaration does not use designated initializer syntax}}
+Bar b4{.b = 1, 2};// expected-error{{variable declaration does not use designated initializer syntax}}
+Bar b5 = {.b = 1, 2}; // expected-error{{variable declaration does not use designated initializer syntax}}
+// The following are valid ways of initializing instances of this struct.
+Bar b6{};
+Bar b7 = {};
+Bar b8{.b = 1};
+Bar b9 = {.b = 1};
+Bar b10{.b = 1, .c = 2};
+Bar b11 = {.b = 1, .c = 2};
+Bar b12 = {.c = 2, .b = 1};
+
+// Struct without require_designated_init attribute
+struct Baz {
+  int d;
+  int e;
+};
+
+// The following are all valid ways of initializing instances of this struct.
+Baz z1;
+Baz z2{};
+Baz z3 = {};
+Baz z4{1, 2};
+Baz z5 = {1, 2};
+Baz z6{.d = 1, .e = 2};
+Baz z7 = {.d = 1, .e = 2};
+Baz z8{1};
+Baz z9 = {1};
+Baz z10{.d = 1, 2};
+Baz z11 = {.d = 1, 2};
+
+// The require_designated_init attribute can also be attached to unions.
+union ATTR Uni { // expected-note 0+ {{required by 'require_designated_init' attribute here}}
+  int x;
+  int y;
+};
+
+// The following are invalid ways of initializing instances of this union.
+Uni u1;   // expected-error{{variable declaration does not use designated initializer syntax}}
+Uni u2{1};// expected-error{{variable declaration does not use designated initializer syntax}}
+Uni u3 = {1}; // expected-error{{variable declaration does not use designated initializer syntax}}
+// The following are valid ways of initializing instances of this union.
+Uni u4{};
+Uni u5 = {};
+Uni u6{.x = 1};
+Uni u7 = {.x = 1};
+
+// The require_designated_init attribute can also be attached to classes.
+class ATTR Cla { // expected-note 0+ {{required by 'require_designated_init' attribute here}}
+public:
+  int x;
+  int y;
+};
+
+// The following are invalid ways of initializing instances of this class.
+Cla c1;   // expected-error{{variable declaration does not use designated initializer syntax}}
+Cla c2{1, 2}; // expected-error{{variable declaration does not use designated initializer syntax}}
+Cla c3 = {1, 2};  // expected-error{{variable declaration does not use designated initializer syntax}}
+Cla c4{.x = 1, 2};// expected-error{{variable declaration does not use designated initializer syntax}}
+Cla c5 = {.x = 1, 2}; // 

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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

C++20 designated initializers don't permit mixing designated fields with 
non-designated ones, so some of the examples here are invalid. However, I think 
we should be looking for an attribute design that works well in both C and C++, 
and with the various Clang extensions that permit the full generality of C 
designated initializers in other language modes. I also think this patch is 
combining multiple features that would be useful to expose separately. To that 
end, I think something like this might make sense:

- An attribute that can be applied to either a field or to a struct that 
requires a designator to be used on any initializer for that field (and for a 
struct, is equivalent to specifying the attribute on all fields)
- An attribute that can be applied to either a field or to a struct that 
requires an explicit initializer to be used for that field, for both aggregate 
initialization and constructor mem-initializer lists (and for a struct, is 
equivalent to specifying the attribute on all fields with no default member 
initializers)

Maybe `requires_designator` and `requires_init` or similar?

And I think these attributes should be made available in both C++11 attribute 
syntax and GNU attribute syntax. Inventing a C++-only extension to improve 
support for designated initializers doesn't seem like the right choice to me.


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

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

[Emmett Neyman via Phabricator via cfe-commits]

emmettneyman updated this revision to Diff 208840.
emmettneyman marked an inline comment as done.
emmettneyman added a comment.

Remove cppreference link, add reference to C++ spec.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64380

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/SemaCXX/attr-designated-init-required.cpp
  clang/test/SemaCXX/attr-require-designated-init.cpp

Index: clang/test/SemaCXX/attr-require-designated-init.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/attr-require-designated-init.cpp
@@ -0,0 +1,104 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+#define ATTR [[clang::require_designated_init]]
+
+// The require_designated_init attribute only applies to types. It will
+// generate a warning when attached to variables, functions, arrays, etc.
+int ATTR x;// expected-error{{'require_designated_init' attribute cannot be applied to types}}
+void ATTR fun(int x) { // expected-error{{'require_designated_init' attribute cannot be applied to types}}
+  return;
+}
+int ATTR arr[10]; // expected-error{{'require_designated_init' attribute cannot be applied to types}}
+
+// Struct with one field with require_designated_init attribute
+struct ATTR Foo { // expected-note 0+ {{required by 'require_designated_init' attribute here}}
+  int a;
+};
+
+// The following are invalid ways of initializing instances of this struct.
+Foo f1;   // expected-error{{variable declaration does not use designated initializer syntax}}
+Foo f2{1};// expected-error{{variable declaration does not use designated initializer syntax}}
+Foo f3 = {1}; // expected-error{{variable declaration does not use designated initializer syntax}}
+// The following are valid ways of initializing instances of this struct.
+Foo f4{};
+Foo f5 = {};
+Foo f6{.a = 1};
+Foo f7 = {.a = 1};
+
+// Struct with multiple fields wth require_designated_init attribute
+struct ATTR Bar { // expected-note 0+ {{required by 'require_designated_init' attribute here}}
+  int b;
+  int c;
+};
+
+// The following are invalid ways of initializing instances of this struct.
+Bar b1;   // expected-error{{variable declaration does not use designated initializer syntax}}
+Bar b2{1, 2}; // expected-error{{variable declaration does not use designated initializer syntax}}
+Bar b3 = {1, 2};  // expected-error{{variable declaration does not use designated initializer syntax}}
+Bar b4{.b = 1, 2};// expected-error{{variable declaration does not use designated initializer syntax}}
+Bar b5 = {.b = 1, 2}; // expected-error{{variable declaration does not use designated initializer syntax}}
+// The following are valid ways of initializing instances of this struct.
+Bar b6{};
+Bar b7 = {};
+Bar b8{.b = 1};
+Bar b9 = {.b = 1};
+Bar b10{.b = 1, .c = 2};
+Bar b11 = {.b = 1, .c = 2};
+Bar b12 = {.c = 2, .b = 1};
+
+// Struct without require_designated_init attribute
+struct Baz {
+  int d;
+  int e;
+};
+
+// The following are all valid ways of initializing instances of this struct.
+Baz z1;
+Baz z2{};
+Baz z3 = {};
+Baz z4{1, 2};
+Baz z5 = {1, 2};
+Baz z6{.d = 1, .e = 2};
+Baz z7 = {.d = 1, .e = 2};
+Baz z8{1};
+Baz z9 = {1};
+Baz z10{.d = 1, 2};
+Baz z11 = {.d = 1, 2};
+
+// The require_designated_init attribute can also be attached to unions.
+union ATTR Uni { // expected-note 0+ {{required by 'require_designated_init' attribute here}}
+  int x;
+  int y;
+};
+
+// The following are invalid ways of initializing instances of this union.
+Uni u1;   // expected-error{{variable declaration does not use designated initializer syntax}}
+Uni u2{1};// expected-error{{variable declaration does not use designated initializer syntax}}
+Uni u3 = {1}; // expected-error{{variable declaration does not use designated initializer syntax}}
+// The following are valid ways of initializing instances of this union.
+Uni u4{};
+Uni u5 = {};
+Uni u6{.x = 1};
+Uni u7 = {.x = 1};
+
+// The require_designated_init attribute can also be attached to classes.
+class ATTR Cla { // expected-note 0+ {{required by 'require_designated_init' attribute here}}
+public:
+  int x;
+  int y;
+};
+
+// The following are invalid ways of initializing instances of this class.
+Cla c1;   // expected-error{{variable declaration does not use designated initializer syntax}}
+Cla c2{1, 2}; // expected-error{{variable declaration does not use designated initializer syntax}}
+Cla c3 = {1, 2};  // expected-error{{variable declaration does not use designated initializer syntax}}
+Cla c4{.x = 1, 2};// expected-error{{variable declaration does not use designated initializer 

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

[Emmett Neyman via Phabricator via cfe-commits]

emmettneyman updated this revision to Diff 208829.
emmettneyman added a comment.

Updated tests and diagnostic messages with new attribute spelling


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64380

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/SemaCXX/attr-designated-init-required.cpp
  clang/test/SemaCXX/attr-require-designated-init.cpp

Index: clang/test/SemaCXX/attr-require-designated-init.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/attr-require-designated-init.cpp
@@ -0,0 +1,104 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+#define ATTR [[clang::require_designated_init]]
+
+// The require_designated_init attribute only applies to types. It will
+// generate a warning when attached to variables, functions, arrays, etc.
+int ATTR x;// expected-error{{'require_designated_init' attribute cannot be applied to types}}
+void ATTR fun(int x) { // expected-error{{'require_designated_init' attribute cannot be applied to types}}
+  return;
+}
+int ATTR arr[10]; // expected-error{{'require_designated_init' attribute cannot be applied to types}}
+
+// Struct with one field with require_designated_init attribute
+struct ATTR Foo { // expected-note 0+ {{required by 'require_designated_init' attribute here}}
+  int a;
+};
+
+// The following are invalid ways of initializing instances of this struct.
+Foo f1;   // expected-error{{variable declaration does not use designated initializer syntax}}
+Foo f2{1};// expected-error{{variable declaration does not use designated initializer syntax}}
+Foo f3 = {1}; // expected-error{{variable declaration does not use designated initializer syntax}}
+// The following are valid ways of initializing instances of this struct.
+Foo f4{};
+Foo f5 = {};
+Foo f6{.a = 1};
+Foo f7 = {.a = 1};
+
+// Struct with multiple fields wth require_designated_init attribute
+struct ATTR Bar { // expected-note 0+ {{required by 'require_designated_init' attribute here}}
+  int b;
+  int c;
+};
+
+// The following are invalid ways of initializing instances of this struct.
+Bar b1;   // expected-error{{variable declaration does not use designated initializer syntax}}
+Bar b2{1, 2}; // expected-error{{variable declaration does not use designated initializer syntax}}
+Bar b3 = {1, 2};  // expected-error{{variable declaration does not use designated initializer syntax}}
+Bar b4{.b = 1, 2};// expected-error{{variable declaration does not use designated initializer syntax}}
+Bar b5 = {.b = 1, 2}; // expected-error{{variable declaration does not use designated initializer syntax}}
+// The following are valid ways of initializing instances of this struct.
+Bar b6{};
+Bar b7 = {};
+Bar b8{.b = 1};
+Bar b9 = {.b = 1};
+Bar b10{.b = 1, .c = 2};
+Bar b11 = {.b = 1, .c = 2};
+Bar b12 = {.c = 2, .b = 1};
+
+// Struct without require_designated_init attribute
+struct Baz {
+  int d;
+  int e;
+};
+
+// The following are all valid ways of initializing instances of this struct.
+Baz z1;
+Baz z2{};
+Baz z3 = {};
+Baz z4{1, 2};
+Baz z5 = {1, 2};
+Baz z6{.d = 1, .e = 2};
+Baz z7 = {.d = 1, .e = 2};
+Baz z8{1};
+Baz z9 = {1};
+Baz z10{.d = 1, 2};
+Baz z11 = {.d = 1, 2};
+
+// The require_designated_init attribute can also be attached to unions.
+union ATTR Uni { // expected-note 0+ {{required by 'require_designated_init' attribute here}}
+  int x;
+  int y;
+};
+
+// The following are invalid ways of initializing instances of this union.
+Uni u1;   // expected-error{{variable declaration does not use designated initializer syntax}}
+Uni u2{1};// expected-error{{variable declaration does not use designated initializer syntax}}
+Uni u3 = {1}; // expected-error{{variable declaration does not use designated initializer syntax}}
+// The following are valid ways of initializing instances of this union.
+Uni u4{};
+Uni u5 = {};
+Uni u6{.x = 1};
+Uni u7 = {.x = 1};
+
+// The require_designated_init attribute can also be attached to classes.
+class ATTR Cla { // expected-note 0+ {{required by 'require_designated_init' attribute here}}
+public:
+  int x;
+  int y;
+};
+
+// The following are invalid ways of initializing instances of this class.
+Cla c1;   // expected-error{{variable declaration does not use designated initializer syntax}}
+Cla c2{1, 2}; // expected-error{{variable declaration does not use designated initializer syntax}}
+Cla c3 = {1, 2};  // expected-error{{variable declaration does not use designated initializer syntax}}
+Cla c4{.x = 1, 2};// expected-error{{variable declaration does not use designated initializer syntax}}
+Cla c5 = {.x = 1, 2}; // 

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

[Emmett Neyman via Phabricator via cfe-commits]

emmettneyman updated this revision to Diff 208814.
emmettneyman marked 3 inline comments as done.
emmettneyman added a comment.

Changed attribute spelling from GNU to CXX11 and renamed the 'required' 
attribute to 'designated_init_required'


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64380

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/SemaCXX/attr-designated-init-required.cpp
  clang/test/SemaCXX/attr-require-designated-init.cpp

Index: clang/test/SemaCXX/attr-require-designated-init.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/attr-require-designated-init.cpp
@@ -0,0 +1,104 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+#define ATTR [[clang::require_designated_init]]
+
+// The require_designated_init attribute only applies to types. It will
+// generate a warning when attached to variables, functions, arrays, etc.
+int ATTR x;// expected-error{{'require_designated_init' attribute cannot be applied to types}}
+void ATTR fun(int x) { // expected-error{{'require_designated_init' attribute cannot be applied to types}}
+  return;
+}
+int ATTR arr[10]; // expected-error{{'require_designated_init' attribute cannot be applied to types}}
+
+// Struct with one field with require_designated_init attribute
+struct ATTR Foo { // expected-note 0+ {{required by 'require_designated_init' attribute here}}
+  int a;
+};
+
+// The following are invalid ways of initializing instances of this struct.
+Foo f1;   // expected-error{{variable declaration does not use designated initializer syntax}}
+Foo f2{1};// expected-error{{variable declaration does not use designated initializer syntax}}
+Foo f3 = {1}; // expected-error{{variable declaration does not use designated initializer syntax}}
+// The following are valid ways of initializing instances of this struct.
+Foo f4{};
+Foo f5 = {};
+Foo f6{.a = 1};
+Foo f7 = {.a = 1};
+
+// Struct with multiple fields wth require_designated_init attribute
+struct ATTR Bar { // expected-note 0+ {{required by 'require_designated_init' attribute here}}
+  int b;
+  int c;
+};
+
+// The following are invalid ways of initializing instances of this struct.
+Bar b1;   // expected-error{{variable declaration does not use designated initializer syntax}}
+Bar b2{1, 2}; // expected-error{{variable declaration does not use designated initializer syntax}}
+Bar b3 = {1, 2};  // expected-error{{variable declaration does not use designated initializer syntax}}
+Bar b4{.b = 1, 2};// expected-error{{variable declaration does not use designated initializer syntax}}
+Bar b5 = {.b = 1, 2}; // expected-error{{variable declaration does not use designated initializer syntax}}
+// The following are valid ways of initializing instances of this struct.
+Bar b6{};
+Bar b7 = {};
+Bar b8{.b = 1};
+Bar b9 = {.b = 1};
+Bar b10{.b = 1, .c = 2};
+Bar b11 = {.b = 1, .c = 2};
+Bar b12 = {.c = 2, .b = 1};
+
+// Struct without require_designated_init attribute
+struct Baz {
+  int d;
+  int e;
+};
+
+// The following are all valid ways of initializing instances of this struct.
+Baz z1;
+Baz z2{};
+Baz z3 = {};
+Baz z4{1, 2};
+Baz z5 = {1, 2};
+Baz z6{.d = 1, .e = 2};
+Baz z7 = {.d = 1, .e = 2};
+Baz z8{1};
+Baz z9 = {1};
+Baz z10{.d = 1, 2};
+Baz z11 = {.d = 1, 2};
+
+// The require_designated_init attribute can also be attached to unions.
+union ATTR Uni { // expected-note 0+ {{required by 'require_designated_init' attribute here}}
+  int x;
+  int y;
+};
+
+// The following are invalid ways of initializing instances of this union.
+Uni u1;   // expected-error{{variable declaration does not use designated initializer syntax}}
+Uni u2{1};// expected-error{{variable declaration does not use designated initializer syntax}}
+Uni u3 = {1}; // expected-error{{variable declaration does not use designated initializer syntax}}
+// The following are valid ways of initializing instances of this union.
+Uni u4{};
+Uni u5 = {};
+Uni u6{.x = 1};
+Uni u7 = {.x = 1};
+
+// The require_designated_init attribute can also be attached to classes.
+class ATTR Cla { // expected-note 0+ {{required by 'require_designated_init' attribute here}}
+public:
+  int x;
+  int y;
+};
+
+// The following are invalid ways of initializing instances of this class.
+Cla c1;   // expected-error{{variable declaration does not use designated initializer syntax}}
+Cla c2{1, 2}; // expected-error{{variable declaration does not use designated initializer syntax}}
+Cla c3 = {1, 2};  // expected-error{{variable declaration does not use designated initializer syntax}}
+Cla c4{.x = 1, 2};// 

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

[Emmett Neyman via Phabricator via cfe-commits]

emmettneyman updated this revision to Diff 208741.
emmettneyman marked 6 inline comments as done.
emmettneyman added a comment.

Small changes in response to feedback


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64380

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/SemaCXX/attr-require-designated-init.cpp
  clang/test/SemaCXX/attr-required.cpp

Index: clang/test/SemaCXX/attr-required.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/attr-required.cpp
@@ -0,0 +1,63 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+#define ATTR __attribute__((required))
+
+int ATTR x;// expected-warning{{'required' attribute only applies to non-static data members}}
+void ATTR fun(int x) { // expected-warning{{'required' attribute only applies to non-static data members}}
+  return;
+}
+struct ATTR Foo { // expected-warning{{'required' attribute only applies to non-static data members}}
+  int x;
+};
+
+// Struct with one required field
+struct Bar {
+  int ATTR y; // expected-note 0+ {{enforced by 'required' attribute here}}
+};
+
+// The following are invalid ways of initializing instances of this struct.
+Bar b1;// expected-error{{initializer for variable b1 must explicitly initialize field y}}
+Bar b2{};  // expected-error{{initializer for variable b2 must explicitly initialize field y}}
+Bar b3{1}; // expected-error{{initializer for variable b3 must explicitly initialize field y}}
+
+// The following are valid ways of initializing instances of this struct.
+Bar b6{.y = 1};
+
+// Struct with multiple required fields
+struct Baz {
+  int ATTR x; // expected-note 0+ {{enforced by 'required' attribute here}}
+  int y;
+  int ATTR z; // expected-note 0+ {{enforced by 'required' attribute here}}
+};
+
+// The following are invalid ways of initializing instances of this struct.
+Baz z1; // expected-error{{initializer for variable z1 must explicitly initialize field x}} expected-error{{initializer for variable z1 must explicitly initialize field z}}
+Baz z2{};   // expected-error{{initializer for variable z2 must explicitly initialize field x}} expected-error{{initializer for variable z2 must explicitly initialize field z}}
+Baz z3{1, 2};   // expected-error{{initializer for variable z3 must explicitly initialize field x}} expected-error{{initializer for variable z3 must explicitly initialize field z}}
+Baz z4{1, 2, 3};// expected-error{{initializer for variable z4 must explicitly initialize field x}} expected-error{{initializer for variable z4 must explicitly initialize field z}}
+Baz z5{.x = 1, 2};  // expected-error{{initializer for variable z5 must explicitly initialize field z}}
+Baz z6{.x = 1, .y = 2}; // expected-error{{initializer for variable z6 must explicitly initialize field z}}
+
+// The following are valid ways of initializing instances of this struct.
+Baz z7{.x = 1, .y = 2, .z = 3};
+Baz z8{.x = 1, .z = 3};
+Baz z9{.x = 1, 2, .z = 3};
+
+// The required attribute can also be applied to public fields of classes.
+class Cla {
+public:
+  int ATTR x; // expected-note 0+ {{enforced by 'required' attribute here}}
+  int y;
+};
+
+// The following are invalid ways of initializing instances of this class.
+Cla c1;// expected-error{{initializer for variable c1 must explicitly initialize field x}}
+Cla c2{};  // expected-error{{initializer for variable c2 must explicitly initialize field x}}
+Cla c3{1}; // expected-error{{initializer for variable c3 must explicitly initialize field x}}
+Cla c4{1, 2};  // expected-error{{initializer for variable c4 must explicitly initialize field x}}
+Cla c5{1, .y = 2}; // expected-error{{initializer for variable c5 must explicitly initialize field x}}
+
+// The following are valid ways of initializing instances of this class.
+Cla c6{.x = 1};
+Cla c7{.x = 1, .y = 2};
+Cla c8{.x = 1, 2};
Index: clang/test/SemaCXX/attr-require-designated-init.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/attr-require-designated-init.cpp
@@ -0,0 +1,104 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+#define ATTR __attribute__((require_designated_init))
+
+// The require_designated_init attribute only applies to types. It will
+// generate a warning when attached to variables, functions, arrays, etc.
+int ATTR x;// expected-warning{{attribute only applies to types}}
+void ATTR fun(int x) { // expected-warning{{attribute only applies to types}}
+  return;
+}
+int ATTR arr[10]; // expected-warning{{attribute only applies to types}}
+
+// Struct with one field with 

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

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:1948
+def RequireDesignatedInit : InheritableAttr {
+  let Spellings = [GNU<"require_designated_init">];
+  let Subjects = SubjectList<[Type]>;

This seems like an extension?  I think that we want to explicitly mark this as 
an extension, as this is not available in GCC AFAIK.



Comment at: clang/include/clang/Basic/Attr.td:1953
+  let Documentation = [RequireDesignatedInitDocs];
+}
+

Is this meant for C or C++?  Given that the docs reference the C++20 designated 
initialization, I suspect that this is meant for C++, in which case, we should 
restrict this to the C++ language.  At which point, perhaps we should restrict 
the spelling to the C++ attribute syntax as well?



Comment at: clang/include/clang/Basic/Attr.td:1956
+def Required : InheritableAttr {
+  let Spellings = [GNU<"required">];
+  let Subjects = SubjectList<[Field]>;

This seems like an extension as well?  I’m not sure that the spelling here is 
good.  It seems overly general.  At the very least, something like 
`__attribute__((__designated_initialization_required__))` seems better.



Comment at: clang/include/clang/Basic/AttrDocs.td:1453
+variable of that struct's type is declared, the declaration uses `designated
+initializer syntax 
`_.
+For a struct ``Foo`` with one integer field ``x``, the following declarations

I think it would be better to reference the specification instead of a link to 
cppreference.  Also, I think its better to use the language agnostic URL.



Comment at: clang/lib/Sema/SemaDecl.cpp:11214
 
+  if (TagDecl *TD = VDecl->getType().getTypePtr()->getAsTagDecl()) {
+// If the type of the declaration is a struct/class and that type has the

I would just use `auto`, you spell out the type in the expression.



Comment at: clang/lib/Sema/SemaDecl.cpp:11218
+// the proper designated initializer syntax.
+if (TD && TD->hasAttr()) {
+  if (InitListExpr *ILE = dyn_cast(Init)) {

`TD &&` is tautologically true.  Please drop it.



Comment at: clang/lib/Sema/SemaDecl.cpp:11219
+if (TD && TD->hasAttr()) {
+  if (InitListExpr *ILE = dyn_cast(Init)) {
+for (unsigned i = 0; i < ILE->getNumInits(); i++) {

I would use `auto` as you spell out the type in the expression.



Comment at: clang/lib/Sema/SemaDecl.cpp:11223
+  DesignatedInitExpr *DIE = dyn_cast(init);
+  if (!DIE) {
+SourceRange SR(VDecl->getSourceRange().getBegin(),

I would just combine this with the previous like and unindent the rest of the 
path by using the following:

```
  if (!isa(init))
continue;
```



Comment at: clang/lib/Sema/SemaDecl.cpp:11238
+}
+// If the type of the declaration is a struct/class, we must check whether
+// any of the fields have the required attribute. If any of them do, we 
must

A new line before this line would be nice.



Comment at: clang/lib/Sema/SemaDecl.cpp:11247
+  std::set RequiredFields;
+  for (auto FD : RD->fields()) {
+if (FD->hasAttr()) {

Any reason that this cannot be `const auto *FD`?


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

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

[Emmett Neyman via Phabricator via cfe-commits]

emmettneyman updated this revision to Diff 208544.
emmettneyman added a comment.

Undo clang-format changes to SemaDecl.cpp and SemaDeclAttr.cpp


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64380

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/SemaCXX/attr-require-designated-init.cpp
  clang/test/SemaCXX/attr-required.cpp

Index: clang/test/SemaCXX/attr-required.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/attr-required.cpp
@@ -0,0 +1,63 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+#define ATTR __attribute__((required))
+
+int ATTR x;// expected-warning{{'required' attribute only applies to non-static data members}}
+void ATTR fun(int x) { // expected-warning{{'required' attribute only applies to non-static data members}}
+  return;
+}
+struct ATTR Foo { // expected-warning{{'required' attribute only applies to non-static data members}}
+  int x;
+};
+
+// Struct with one required field
+struct Bar {
+  int ATTR y; // expected-note 0+ {{enforced by 'required' attribute here}}
+};
+
+// The following are invalid ways of initializing instances of this struct.
+Bar b1;// expected-error{{initializer for variable b1 must explicitly initialize field y}}
+Bar b2{};  // expected-error{{initializer for variable b2 must explicitly initialize field y}}
+Bar b3{1}; // expected-error{{initializer for variable b3 must explicitly initialize field y}}
+
+// The following are valid ways of initializing instances of this struct.
+Bar b6{.y = 1};
+
+// Struct with multiple required fields
+struct Baz {
+  int ATTR x; // expected-note 0+ {{enforced by 'required' attribute here}}
+  int y;
+  int ATTR z; // expected-note 0+ {{enforced by 'required' attribute here}}
+};
+
+// The following are invalid ways of initializing instances of this struct.
+Baz z1; // expected-error{{initializer for variable z1 must explicitly initialize field x}} expected-error{{initializer for variable z1 must explicitly initialize field z}}
+Baz z2{};   // expected-error{{initializer for variable z2 must explicitly initialize field x}} expected-error{{initializer for variable z2 must explicitly initialize field z}}
+Baz z3{1, 2};   // expected-error{{initializer for variable z3 must explicitly initialize field x}} expected-error{{initializer for variable z3 must explicitly initialize field z}}
+Baz z4{1, 2, 3};// expected-error{{initializer for variable z4 must explicitly initialize field x}} expected-error{{initializer for variable z4 must explicitly initialize field z}}
+Baz z5{.x = 1, 2};  // expected-error{{initializer for variable z5 must explicitly initialize field z}}
+Baz z6{.x = 1, .y = 2}; // expected-error{{initializer for variable z6 must explicitly initialize field z}}
+
+// The following are valid ways of initializing instances of this struct.
+Baz z7{.x = 1, .y = 2, .z = 3};
+Baz z8{.x = 1, .z = 3};
+Baz z9{.x = 1, 2, .z = 3};
+
+// The required attribute can also be applied to public fields of classes.
+class Cla {
+public:
+  int ATTR x; // expected-note 0+ {{enforced by 'required' attribute here}}
+  int y;
+};
+
+// The following are invalid ways of initializing instances of this class.
+Cla c1;// expected-error{{initializer for variable c1 must explicitly initialize field x}}
+Cla c2{};  // expected-error{{initializer for variable c2 must explicitly initialize field x}}
+Cla c3{1}; // expected-error{{initializer for variable c3 must explicitly initialize field x}}
+Cla c4{1, 2};  // expected-error{{initializer for variable c4 must explicitly initialize field x}}
+Cla c5{1, .y = 2}; // expected-error{{initializer for variable c5 must explicitly initialize field x}}
+
+// The following are valid ways of initializing instances of this class.
+Cla c6{.x = 1};
+Cla c7{.x = 1, .y = 2};
+Cla c8{.x = 1, 2};
Index: clang/test/SemaCXX/attr-require-designated-init.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/attr-require-designated-init.cpp
@@ -0,0 +1,104 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+#define ATTR __attribute__((require_designated_init))
+
+// The require_designated_init attribute only applies to types. It will
+// generate a warning when attached to variables, functions, arrays, etc.
+int ATTR x;// expected-warning{{attribute only applies to types}}
+void ATTR fun(int x) { // expected-warning{{attribute only applies to types}}
+  return;
+}
+int ATTR arr[10]; // expected-warning{{attribute only applies to types}}
+
+// Struct with one field with require_designated_init