c++: Fixing the wording of () aggregate-init [PR92812]

Message ID 20200720232835.47157-1-polacek@redhat.com
State New
Headers show
Series
  • c++: Fixing the wording of () aggregate-init [PR92812]
Related show

Commit Message

Jonathan Wakely via Gcc-patches July 20, 2020, 11:28 p.m.
P1975R0 tweaks the static_cast wording: it says that "An expression e can be
explicitly converted to a type T if [...] T is an aggregate type having a first
element x and there is an implicit conversion sequence from e to the type of
x."  This already works for classes, e.g.:

  struct Aggr { int x; int y; };
  Aggr a = static_cast<Aggr>(1);

albeit I noticed a -Wmissing-field-initializer warning which is unlikely to be
helpful in this context, as there's nothing like static_cast<Aggr>(1, 2)
to quash that warning.

However, the proposal also mentions "If T is ``array of unknown bound of U'',
this direct-initialization defines the type of the expression as U[1]" which
suggest that this should work for arrays (they're aggregates too, after all).
Ville, can you confirm that these

  int (&&r)[3] = static_cast<int[3]>(42);
  int (&&r2)[1] = static_cast<int[]>(42);

are supposed to work now?  There's no {} variant to check.  Thanks.

Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?

gcc/cp/ChangeLog:

	PR c++/92812
	* typeck.c (build_static_cast_1): Add warning_sentinel for
	-Wmissing-field-initializer.

gcc/testsuite/ChangeLog:

	PR c++/92812
	* g++.dg/cpp2a/paren-init27.C: New test.
---
 gcc/cp/typeck.c                           |  3 +++
 gcc/testsuite/g++.dg/cpp2a/paren-init27.C | 24 +++++++++++++++++++++++
 2 files changed, 27 insertions(+)
 create mode 100644 gcc/testsuite/g++.dg/cpp2a/paren-init27.C


base-commit: 932fbc868ad429167a3d4d5625aa9d6dc0b4506b
-- 
2.26.2

Comments

Jonathan Wakely via Gcc-patches July 21, 2020, 9:53 a.m. | #1
On Tue, 21 Jul 2020 at 02:28, Marek Polacek <polacek@redhat.com> wrote:
>

> P1975R0 tweaks the static_cast wording: it says that "An expression e can be

> explicitly converted to a type T if [...] T is an aggregate type having a first

> element x and there is an implicit conversion sequence from e to the type of

> x."  This already works for classes, e.g.:

>

>   struct Aggr { int x; int y; };

>   Aggr a = static_cast<Aggr>(1);

>

> albeit I noticed a -Wmissing-field-initializer warning which is unlikely to be

> helpful in this context, as there's nothing like static_cast<Aggr>(1, 2)

> to quash that warning.

>

> However, the proposal also mentions "If T is ``array of unknown bound of U'',

> this direct-initialization defines the type of the expression as U[1]" which

> suggest that this should work for arrays (they're aggregates too, after all).

> Ville, can you confirm that these

>

>   int (&&r)[3] = static_cast<int[3]>(42);

>   int (&&r2)[1] = static_cast<int[]>(42);

>

> are supposed to work now?  There's no {} variant to check.  Thanks.


I don't know what it means to cast something to an array; doesn't that create
an array prvalue? Is that a thing?
Jonathan Wakely via Gcc-patches July 21, 2020, 6:56 p.m. | #2
On Tue, Jul 21, 2020 at 12:53:03PM +0300, Ville Voutilainen wrote:
> On Tue, 21 Jul 2020 at 02:28, Marek Polacek <polacek@redhat.com> wrote:

> >

> > P1975R0 tweaks the static_cast wording: it says that "An expression e can be

> > explicitly converted to a type T if [...] T is an aggregate type having a first

> > element x and there is an implicit conversion sequence from e to the type of

> > x."  This already works for classes, e.g.:

> >

> >   struct Aggr { int x; int y; };

> >   Aggr a = static_cast<Aggr>(1);

> >

> > albeit I noticed a -Wmissing-field-initializer warning which is unlikely to be

> > helpful in this context, as there's nothing like static_cast<Aggr>(1, 2)

> > to quash that warning.

> >

> > However, the proposal also mentions "If T is ``array of unknown bound of U'',

> > this direct-initialization defines the type of the expression as U[1]" which

> > suggest that this should work for arrays (they're aggregates too, after all).

> > Ville, can you confirm that these

> >

> >   int (&&r)[3] = static_cast<int[3]>(42);

> >   int (&&r2)[1] = static_cast<int[]>(42);

> >

> > are supposed to work now?  There's no {} variant to check.  Thanks.

> 

> I don't know what it means to cast something to an array; doesn't that create

> an array prvalue? Is that a thing?


Yes, I imagined this would be similar to

using T = int[3];
int (&&r)[3] = T{1, 2, 3}; // binds to array prvalue, lifetime extended

but I'd like to avoid allowing code that isn't supposed to work.
We also might want to consider if we want to extend the lifetime of r/r2 in my
previous example (I think so; DRs 1299/1376).  Worth bothering CWG?

Marek
Jonathan Wakely via Gcc-patches July 21, 2020, 7:03 p.m. | #3
On Tue, 21 Jul 2020 at 21:56, Marek Polacek <polacek@redhat.com> wrote:
>

> On Tue, Jul 21, 2020 at 12:53:03PM +0300, Ville Voutilainen wrote:

> > On Tue, 21 Jul 2020 at 02:28, Marek Polacek <polacek@redhat.com> wrote:

> > >

> > > P1975R0 tweaks the static_cast wording: it says that "An expression e can be

> > > explicitly converted to a type T if [...] T is an aggregate type having a first

> > > element x and there is an implicit conversion sequence from e to the type of

> > > x."  This already works for classes, e.g.:

> > >

> > >   struct Aggr { int x; int y; };

> > >   Aggr a = static_cast<Aggr>(1);

> > >

> > > albeit I noticed a -Wmissing-field-initializer warning which is unlikely to be

> > > helpful in this context, as there's nothing like static_cast<Aggr>(1, 2)

> > > to quash that warning.

> > >

> > > However, the proposal also mentions "If T is ``array of unknown bound of U'',

> > > this direct-initialization defines the type of the expression as U[1]" which

> > > suggest that this should work for arrays (they're aggregates too, after all).

> > > Ville, can you confirm that these

> > >

> > >   int (&&r)[3] = static_cast<int[3]>(42);

> > >   int (&&r2)[1] = static_cast<int[]>(42);

> > >

> > > are supposed to work now?  There's no {} variant to check.  Thanks.

> >

> > I don't know what it means to cast something to an array; doesn't that create

> > an array prvalue? Is that a thing?

>

> Yes, I imagined this would be similar to

>

> using T = int[3];

> int (&&r)[3] = T{1, 2, 3}; // binds to array prvalue, lifetime extended

>

> but I'd like to avoid allowing code that isn't supposed to work.


Okay. I think it's remotely reasonable that a static_cast<T>(42) would
work for an array, then.
And by a natural extension, that using the concrete type would also
work. That seems consistent,
but doesn't seem like it rises to the level of "an important part of
the design". Trafficking arrays
around in generic code is already hard, because they don't behave like
other things do, so the
answer to "was this supposed to work?" is pretty much "it wasn't
considered during the design phase".

> We also might want to consider if we want to extend the lifetime of r/r2 in my

> previous example (I think so; DRs 1299/1376).  Worth bothering CWG?


I think it's a good idea to bother CWG with both the above and the
lifetime extension.
Jonathan Wakely via Gcc-patches July 21, 2020, 7:38 p.m. | #4
On Tue, Jul 21, 2020 at 10:03:37PM +0300, Ville Voutilainen via Gcc-patches wrote:
> On Tue, 21 Jul 2020 at 21:56, Marek Polacek <polacek@redhat.com> wrote:

> >

> > On Tue, Jul 21, 2020 at 12:53:03PM +0300, Ville Voutilainen wrote:

> > > On Tue, 21 Jul 2020 at 02:28, Marek Polacek <polacek@redhat.com> wrote:

> > > >

> > > > P1975R0 tweaks the static_cast wording: it says that "An expression e can be

> > > > explicitly converted to a type T if [...] T is an aggregate type having a first

> > > > element x and there is an implicit conversion sequence from e to the type of

> > > > x."  This already works for classes, e.g.:

> > > >

> > > >   struct Aggr { int x; int y; };

> > > >   Aggr a = static_cast<Aggr>(1);

> > > >

> > > > albeit I noticed a -Wmissing-field-initializer warning which is unlikely to be

> > > > helpful in this context, as there's nothing like static_cast<Aggr>(1, 2)

> > > > to quash that warning.

> > > >

> > > > However, the proposal also mentions "If T is ``array of unknown bound of U'',

> > > > this direct-initialization defines the type of the expression as U[1]" which

> > > > suggest that this should work for arrays (they're aggregates too, after all).

> > > > Ville, can you confirm that these

> > > >

> > > >   int (&&r)[3] = static_cast<int[3]>(42);

> > > >   int (&&r2)[1] = static_cast<int[]>(42);

> > > >

> > > > are supposed to work now?  There's no {} variant to check.  Thanks.

> > >

> > > I don't know what it means to cast something to an array; doesn't that create

> > > an array prvalue? Is that a thing?

> >

> > Yes, I imagined this would be similar to

> >

> > using T = int[3];

> > int (&&r)[3] = T{1, 2, 3}; // binds to array prvalue, lifetime extended

> >

> > but I'd like to avoid allowing code that isn't supposed to work.

> 

> Okay. I think it's remotely reasonable that a static_cast<T>(42) would

> work for an array, then.

> And by a natural extension, that using the concrete type would also

> work. That seems consistent,

> but doesn't seem like it rises to the level of "an important part of

> the design". Trafficking arrays

> around in generic code is already hard, because they don't behave like

> other things do, so the

> answer to "was this supposed to work?" is pretty much "it wasn't

> considered during the design phase".


Fair enough.

> > We also might want to consider if we want to extend the lifetime of r/r2 in my

> > previous example (I think so; DRs 1299/1376).  Worth bothering CWG?

> 

> I think it's a good idea to bother CWG with both the above and the

> lifetime extension.


I'll email core then.  Thanks for responding!

Jason, I think the patch still makes sense as-is; presumably we can follow up
with the controversial bits (arrays) later.

Marek
Jonathan Wakely via Gcc-patches July 21, 2020, 7:52 p.m. | #5
On Tue, 21 Jul 2020 at 22:39, Marek Polacek <polacek@redhat.com> wrote:
> > Okay. I think it's remotely reasonable that a static_cast<T>(42) would

> > work for an array, then.

> > And by a natural extension, that using the concrete type would also

> > work. That seems consistent,

> > but doesn't seem like it rises to the level of "an important part of

> > the design". Trafficking arrays

> > around in generic code is already hard, because they don't behave like

> > other things do, so the

> > answer to "was this supposed to work?" is pretty much "it wasn't

> > considered during the design phase".

>

> Fair enough.


Sorry. :)  None of the main driving-use-cases for allowing paren-init
of aggregates involved
arrays. They were to some extent expected to hitch a ride since they
are aggregates, but
otherwise they never received particular design-level lovin'. It
seemed plausible to
be able to paren-init a member array in a ctor-initializer.
Paren-initializing automatic storage
duration variables requires a typedef anyway. So the casting cases
have not been fully
fledged design-wise, and that's mostly just oversight. Or lack of
energy, if you wish.
Jonathan Wakely via Gcc-patches July 29, 2020, 11:21 p.m. | #6
On 7/20/20 7:28 PM, Marek Polacek wrote:
> P1975R0 tweaks the static_cast wording: it says that "An expression e can be

> explicitly converted to a type T if [...] T is an aggregate type having a first

> element x and there is an implicit conversion sequence from e to the type of

> x."  This already works for classes, e.g.:

> 

>    struct Aggr { int x; int y; };

>    Aggr a = static_cast<Aggr>(1);

> 

> albeit I noticed a -Wmissing-field-initializer warning which is unlikely to be

> helpful in this context, as there's nothing like static_cast<Aggr>(1, 2)

> to quash that warning.


You could write Aggr{1,2} instead?

This warning could be helpful if they didn't realize that what they were 
writing is initializing one element of an aggregate.

> However, the proposal also mentions "If T is ``array of unknown bound of U'',

> this direct-initialization defines the type of the expression as U[1]" which

> suggest that this should work for arrays (they're aggregates too, after all).

> Ville, can you confirm that these

> 

>    int (&&r)[3] = static_cast<int[3]>(42);

>    int (&&r2)[1] = static_cast<int[]>(42);

> 

> are supposed to work now?  There's no {} variant to check.  Thanks.


I'll review the discussion of this later.

> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?

> 

> gcc/cp/ChangeLog:

> 

> 	PR c++/92812

> 	* typeck.c (build_static_cast_1): Add warning_sentinel for

> 	-Wmissing-field-initializer.

> 

> gcc/testsuite/ChangeLog:

> 

> 	PR c++/92812

> 	* g++.dg/cpp2a/paren-init27.C: New test.

> ---

>   gcc/cp/typeck.c                           |  3 +++

>   gcc/testsuite/g++.dg/cpp2a/paren-init27.C | 24 +++++++++++++++++++++++

>   2 files changed, 27 insertions(+)

>   create mode 100644 gcc/testsuite/g++.dg/cpp2a/paren-init27.C

> 

> diff --git a/gcc/cp/typeck.c b/gcc/cp/typeck.c

> index 589e014f855..062751a6379 100644

> --- a/gcc/cp/typeck.c

> +++ b/gcc/cp/typeck.c

> @@ -7472,6 +7472,9 @@ build_static_cast_1 (location_t loc, tree type, tree expr, bool c_cast_p,

>        static_cast of the form static_cast<T>(e) if the declaration T

>        t(e);" is well-formed, for some invented temporary variable

>        t.  */

> +

> +  /* In C++20, we don't want to warn about static_cast<Aggr>(1).  */

> +  warning_sentinel w (warn_missing_field_initializers);

>     result = perform_direct_initialization_if_possible (type, expr,

>   						      c_cast_p, complain);

>     if (result)

> diff --git a/gcc/testsuite/g++.dg/cpp2a/paren-init27.C b/gcc/testsuite/g++.dg/cpp2a/paren-init27.C

> new file mode 100644

> index 00000000000..a856c7fd7be

> --- /dev/null

> +++ b/gcc/testsuite/g++.dg/cpp2a/paren-init27.C

> @@ -0,0 +1,24 @@

> +// PR c++/92812

> +// P1975R0

> +// { dg-do run { target c++20 } }

> +// { dg-options "-Wall -Wextra" }

> +

> +struct Aggr { int x; int y; };

> +struct Base { int i; Base(int i_) : i{i_} { } };

> +struct BaseAggr : Base { };

> +struct X { };

> +struct AggrSDM { static X x; int i; int j; };

> +

> +int

> +main ()

> +{

> +  Aggr a = static_cast<Aggr>(42);

> +  if (a.x != 42 || a.y != 0)

> +    __builtin_abort ();

> +  BaseAggr b = static_cast<BaseAggr>(42);

> +  if (b.i != 42)

> +    __builtin_abort ();

> +  AggrSDM s = static_cast<AggrSDM>(42);

> +  if (s.i != 42 || s.j != 0)

> +    __builtin_abort ();

> +}

> 

> base-commit: 932fbc868ad429167a3d4d5625aa9d6dc0b4506b

>
Jonathan Wakely via Gcc-patches July 31, 2020, 5:24 p.m. | #7
On Wed, Jul 29, 2020 at 07:21:46PM -0400, Jason Merrill via Gcc-patches wrote:
> On 7/20/20 7:28 PM, Marek Polacek wrote:

> > P1975R0 tweaks the static_cast wording: it says that "An expression e can be

> > explicitly converted to a type T if [...] T is an aggregate type having a first

> > element x and there is an implicit conversion sequence from e to the type of

> > x."  This already works for classes, e.g.:

> > 

> >    struct Aggr { int x; int y; };

> >    Aggr a = static_cast<Aggr>(1);

> > 

> > albeit I noticed a -Wmissing-field-initializer warning which is unlikely to be

> > helpful in this context, as there's nothing like static_cast<Aggr>(1, 2)

> > to quash that warning.

> 

> You could write Aggr{1,2} instead?

> 

> This warning could be helpful if they didn't realize that what they were

> writing is initializing one element of an aggregate.


Ok, I can drop it.  Though explicit cast usually suppress warnings like that.

> > However, the proposal also mentions "If T is ``array of unknown bound of U'',

> > this direct-initialization defines the type of the expression as U[1]" which

> > suggest that this should work for arrays (they're aggregates too, after all).

> > Ville, can you confirm that these

> > 

> >    int (&&r)[3] = static_cast<int[3]>(42);

> >    int (&&r2)[1] = static_cast<int[]>(42);

> > 

> > are supposed to work now?  There's no {} variant to check.  Thanks.

> 

> I'll review the discussion of this later.


It's become clear that the array cases are supposed to work, so I think
let's drop this patch and I'll post a new patch when I get the array cases
working.

Thanks,
Marek

Patch

diff --git a/gcc/cp/typeck.c b/gcc/cp/typeck.c
index 589e014f855..062751a6379 100644
--- a/gcc/cp/typeck.c
+++ b/gcc/cp/typeck.c
@@ -7472,6 +7472,9 @@  build_static_cast_1 (location_t loc, tree type, tree expr, bool c_cast_p,
      static_cast of the form static_cast<T>(e) if the declaration T
      t(e);" is well-formed, for some invented temporary variable
      t.  */
+
+  /* In C++20, we don't want to warn about static_cast<Aggr>(1).  */
+  warning_sentinel w (warn_missing_field_initializers);
   result = perform_direct_initialization_if_possible (type, expr,
 						      c_cast_p, complain);
   if (result)
diff --git a/gcc/testsuite/g++.dg/cpp2a/paren-init27.C b/gcc/testsuite/g++.dg/cpp2a/paren-init27.C
new file mode 100644
index 00000000000..a856c7fd7be
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp2a/paren-init27.C
@@ -0,0 +1,24 @@ 
+// PR c++/92812
+// P1975R0
+// { dg-do run { target c++20 } }
+// { dg-options "-Wall -Wextra" }
+
+struct Aggr { int x; int y; };
+struct Base { int i; Base(int i_) : i{i_} { } };
+struct BaseAggr : Base { };
+struct X { };
+struct AggrSDM { static X x; int i; int j; };
+
+int
+main ()
+{
+  Aggr a = static_cast<Aggr>(42);
+  if (a.x != 42 || a.y != 0)
+    __builtin_abort ();
+  BaseAggr b = static_cast<BaseAggr>(42);
+  if (b.i != 42)
+    __builtin_abort ();
+  AggrSDM s = static_cast<AggrSDM>(42);
+  if (s.i != 42 || s.j != 0)
+    __builtin_abort ();
+}