c++: Implement P1957R2, T* to bool should be considered narrowing.

Message ID 20200221191426.207169-1-polacek@redhat.com
State New
Headers show
Series
  • c++: Implement P1957R2, T* to bool should be considered narrowing.
Related show

Commit Message

Marek Polacek Feb. 21, 2020, 7:14 p.m.
This was approved in the Prague 2020 WG21 meeting so let's adjust the
comment.  Since it's supposed to be a DR I think we should no longer
limit it to C++20.

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

2020-02-21  Marek Polacek  <polacek@redhat.com>

	P1957R2
	* typeck2.c (check_narrowing): Consider T* to bool narrowing
	in C++11 and up.

	* g++.dg/cpp0x/initlist92.C: Don't expect an error in C++20 only.
---
 gcc/cp/typeck2.c                        | 7 ++++---
 gcc/testsuite/g++.dg/cpp0x/initlist92.C | 2 +-
 2 files changed, 5 insertions(+), 4 deletions(-)


base-commit: dbfba41e95d1d93b17e907b7f516b52ed3a3c415
-- 
Marek Polacek • Red Hat, Inc. • 300 A St, Boston, MA

Comments

Marek Polacek Feb. 21, 2020, 7:20 p.m. | #1
On Fri, Feb 21, 2020 at 02:14:26PM -0500, Marek Polacek wrote:
> This was approved in the Prague 2020 WG21 meeting so let's adjust the

> comment.  Since it's supposed to be a DR I think we should no longer

> limit it to C++20.


Which is what clang++ trunk does.

Marek
Jason Merrill Feb. 24, 2020, 2:58 p.m. | #2
On 2/21/20 2:14 PM, Marek Polacek wrote:
> This was approved in the Prague 2020 WG21 meeting so let's adjust the

> comment.  Since it's supposed to be a DR I think we should no longer

> limit it to C++20.


I'm a bit nervous about the impact, but OK.  It's easy enough to turn 
off -Wnarrowing if it's a problem for users.

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

> 

> 2020-02-21  Marek Polacek  <polacek@redhat.com>

> 

> 	P1957R2

> 	* typeck2.c (check_narrowing): Consider T* to bool narrowing

> 	in C++11 and up.

> 

> 	* g++.dg/cpp0x/initlist92.C: Don't expect an error in C++20 only.

> ---

>   gcc/cp/typeck2.c                        | 7 ++++---

>   gcc/testsuite/g++.dg/cpp0x/initlist92.C | 2 +-

>   2 files changed, 5 insertions(+), 4 deletions(-)

> 

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

> index 48920894b3b..68bc2e5c170 100644

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

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

> @@ -1036,9 +1036,10 @@ check_narrowing (tree type, tree init, tsubst_flags_t complain,

>       }

>     else if (TREE_CODE (type) == BOOLEAN_TYPE

>   	   && (TYPE_PTR_P (ftype) || TYPE_PTRMEM_P (ftype)))

> -    /* This hasn't actually made it into C++20 yet, but let's add it now to get

> -       an idea of the impact.  */

> -    ok = (cxx_dialect < cxx2a);

> +    /* C++20 P1957R2: converting from a pointer type or a pointer-to-member

> +       type to bool should be considered narrowing.  This is a DR so is not

> +       limited to C++20 only.  */

> +    ok = false;

>   

>     bool almost_ok = ok;

>     if (!ok && !CONSTANT_CLASS_P (init) && (complain & tf_warning_or_error))

> diff --git a/gcc/testsuite/g++.dg/cpp0x/initlist92.C b/gcc/testsuite/g++.dg/cpp0x/initlist92.C

> index 319264ae274..213b192d441 100644

> --- a/gcc/testsuite/g++.dg/cpp0x/initlist92.C

> +++ b/gcc/testsuite/g++.dg/cpp0x/initlist92.C

> @@ -23,7 +23,7 @@ bool Test4(std::initializer_list<std::string>);

>   

>   int main ()

>   {

> -  ( Test1({"false"}) );	// { dg-error "narrowing" "" { target c++2a } }

> +  ( Test1({"false"}) );	// { dg-error "narrowing" }

>     ( Test2({123}) );

>     ( Test3({456}) );

>     ( Test4({"false"}) );

> 

> base-commit: dbfba41e95d1d93b17e907b7f516b52ed3a3c415

>
Jason Merrill Feb. 24, 2020, 2:59 p.m. | #3
On 2/24/20 9:58 AM, Jason Merrill wrote:
> On 2/21/20 2:14 PM, Marek Polacek wrote:

>> This was approved in the Prague 2020 WG21 meeting so let's adjust the

>> comment.  Since it's supposed to be a DR I think we should no longer

>> limit it to C++20.

> 

> I'm a bit nervous about the impact, but OK.  It's easy enough to turn 

> off -Wnarrowing if it's a problem for users.


Hmm, have you tried doing a Fedora build with this change?

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

>>

>> 2020-02-21  Marek Polacek  <polacek@redhat.com>

>>

>>     P1957R2

>>     * typeck2.c (check_narrowing): Consider T* to bool narrowing

>>     in C++11 and up.

>>

>>     * g++.dg/cpp0x/initlist92.C: Don't expect an error in C++20 only.

>> ---

>>   gcc/cp/typeck2.c                        | 7 ++++---

>>   gcc/testsuite/g++.dg/cpp0x/initlist92.C | 2 +-

>>   2 files changed, 5 insertions(+), 4 deletions(-)

>>

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

>> index 48920894b3b..68bc2e5c170 100644

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

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

>> @@ -1036,9 +1036,10 @@ check_narrowing (tree type, tree init, 

>> tsubst_flags_t complain,

>>       }

>>     else if (TREE_CODE (type) == BOOLEAN_TYPE

>>          && (TYPE_PTR_P (ftype) || TYPE_PTRMEM_P (ftype)))

>> -    /* This hasn't actually made it into C++20 yet, but let's add it 

>> now to get

>> -       an idea of the impact.  */

>> -    ok = (cxx_dialect < cxx2a);

>> +    /* C++20 P1957R2: converting from a pointer type or a 

>> pointer-to-member

>> +       type to bool should be considered narrowing.  This is a DR so 

>> is not

>> +       limited to C++20 only.  */

>> +    ok = false;

>>     bool almost_ok = ok;

>>     if (!ok && !CONSTANT_CLASS_P (init) && (complain & 

>> tf_warning_or_error))

>> diff --git a/gcc/testsuite/g++.dg/cpp0x/initlist92.C 

>> b/gcc/testsuite/g++.dg/cpp0x/initlist92.C

>> index 319264ae274..213b192d441 100644

>> --- a/gcc/testsuite/g++.dg/cpp0x/initlist92.C

>> +++ b/gcc/testsuite/g++.dg/cpp0x/initlist92.C

>> @@ -23,7 +23,7 @@ bool Test4(std::initializer_list<std::string>);

>>   int main ()

>>   {

>> -  ( Test1({"false"}) );    // { dg-error "narrowing" "" { target 

>> c++2a } }

>> +  ( Test1({"false"}) );    // { dg-error "narrowing" }

>>     ( Test2({123}) );

>>     ( Test3({456}) );

>>     ( Test4({"false"}) );

>>

>> base-commit: dbfba41e95d1d93b17e907b7f516b52ed3a3c415

>>

>
Marek Polacek Feb. 24, 2020, 3:19 p.m. | #4
On Mon, Feb 24, 2020 at 09:59:51AM -0500, Jason Merrill wrote:
> On 2/24/20 9:58 AM, Jason Merrill wrote:

> > On 2/21/20 2:14 PM, Marek Polacek wrote:

> > > This was approved in the Prague 2020 WG21 meeting so let's adjust the

> > > comment.  Since it's supposed to be a DR I think we should no longer

> > > limit it to C++20.

> > 

> > I'm a bit nervous about the impact, but OK.  It's easy enough to turn

> > off -Wnarrowing if it's a problem for users.

> 

> Hmm, have you tried doing a Fedora build with this change?


I'll admit I have not.

Jeff, would it be possible to apply this patch onto one of your testers?
We'd be much more comfortable going with this change then.

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

> > > 

> > > 2020-02-21  Marek Polacek  <polacek@redhat.com>

> > > 

> > >     P1957R2

> > >     * typeck2.c (check_narrowing): Consider T* to bool narrowing

> > >     in C++11 and up.

> > > 

> > >     * g++.dg/cpp0x/initlist92.C: Don't expect an error in C++20 only.

> > > ---

> > >   gcc/cp/typeck2.c                        | 7 ++++---

> > >   gcc/testsuite/g++.dg/cpp0x/initlist92.C | 2 +-

> > >   2 files changed, 5 insertions(+), 4 deletions(-)

> > > 

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

> > > index 48920894b3b..68bc2e5c170 100644

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

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

> > > @@ -1036,9 +1036,10 @@ check_narrowing (tree type, tree init,

> > > tsubst_flags_t complain,

> > >       }

> > >     else if (TREE_CODE (type) == BOOLEAN_TYPE

> > >          && (TYPE_PTR_P (ftype) || TYPE_PTRMEM_P (ftype)))

> > > -    /* This hasn't actually made it into C++20 yet, but let's add

> > > it now to get

> > > -       an idea of the impact.  */

> > > -    ok = (cxx_dialect < cxx2a);

> > > +    /* C++20 P1957R2: converting from a pointer type or a

> > > pointer-to-member

> > > +       type to bool should be considered narrowing.  This is a DR

> > > so is not

> > > +       limited to C++20 only.  */

> > > +    ok = false;

> > >     bool almost_ok = ok;

> > >     if (!ok && !CONSTANT_CLASS_P (init) && (complain &

> > > tf_warning_or_error))

> > > diff --git a/gcc/testsuite/g++.dg/cpp0x/initlist92.C

> > > b/gcc/testsuite/g++.dg/cpp0x/initlist92.C

> > > index 319264ae274..213b192d441 100644

> > > --- a/gcc/testsuite/g++.dg/cpp0x/initlist92.C

> > > +++ b/gcc/testsuite/g++.dg/cpp0x/initlist92.C

> > > @@ -23,7 +23,7 @@ bool Test4(std::initializer_list<std::string>);

> > >   int main ()

> > >   {

> > > -  ( Test1({"false"}) );    // { dg-error "narrowing" "" { target

> > > c++2a } }

> > > +  ( Test1({"false"}) );    // { dg-error "narrowing" }

> > >     ( Test2({123}) );

> > >     ( Test3({456}) );

> > >     ( Test4({"false"}) );

> > > 

> > > base-commit: dbfba41e95d1d93b17e907b7f516b52ed3a3c415

> > > 

> > 

> 


Marek
Marek Polacek March 2, 2020, 3:45 p.m. | #5
Ping: Jeff, see the question below.  Thanks.

On Mon, Feb 24, 2020 at 10:19:20AM -0500, Marek Polacek wrote:
> On Mon, Feb 24, 2020 at 09:59:51AM -0500, Jason Merrill wrote:

> > On 2/24/20 9:58 AM, Jason Merrill wrote:

> > > On 2/21/20 2:14 PM, Marek Polacek wrote:

> > > > This was approved in the Prague 2020 WG21 meeting so let's adjust the

> > > > comment.  Since it's supposed to be a DR I think we should no longer

> > > > limit it to C++20.

> > > 

> > > I'm a bit nervous about the impact, but OK.  It's easy enough to turn

> > > off -Wnarrowing if it's a problem for users.

> > 

> > Hmm, have you tried doing a Fedora build with this change?

> 

> I'll admit I have not.

> 

> Jeff, would it be possible to apply this patch onto one of your testers?

> We'd be much more comfortable going with this change then.

> 

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

> > > > 

> > > > 2020-02-21  Marek Polacek  <polacek@redhat.com>

> > > > 

> > > >     P1957R2

> > > >     * typeck2.c (check_narrowing): Consider T* to bool narrowing

> > > >     in C++11 and up.

> > > > 

> > > >     * g++.dg/cpp0x/initlist92.C: Don't expect an error in C++20 only.

> > > > ---

> > > >   gcc/cp/typeck2.c                        | 7 ++++---

> > > >   gcc/testsuite/g++.dg/cpp0x/initlist92.C | 2 +-

> > > >   2 files changed, 5 insertions(+), 4 deletions(-)

> > > > 

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

> > > > index 48920894b3b..68bc2e5c170 100644

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

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

> > > > @@ -1036,9 +1036,10 @@ check_narrowing (tree type, tree init,

> > > > tsubst_flags_t complain,

> > > >       }

> > > >     else if (TREE_CODE (type) == BOOLEAN_TYPE

> > > >          && (TYPE_PTR_P (ftype) || TYPE_PTRMEM_P (ftype)))

> > > > -    /* This hasn't actually made it into C++20 yet, but let's add

> > > > it now to get

> > > > -       an idea of the impact.  */

> > > > -    ok = (cxx_dialect < cxx2a);

> > > > +    /* C++20 P1957R2: converting from a pointer type or a

> > > > pointer-to-member

> > > > +       type to bool should be considered narrowing.  This is a DR

> > > > so is not

> > > > +       limited to C++20 only.  */

> > > > +    ok = false;

> > > >     bool almost_ok = ok;

> > > >     if (!ok && !CONSTANT_CLASS_P (init) && (complain &

> > > > tf_warning_or_error))

> > > > diff --git a/gcc/testsuite/g++.dg/cpp0x/initlist92.C

> > > > b/gcc/testsuite/g++.dg/cpp0x/initlist92.C

> > > > index 319264ae274..213b192d441 100644

> > > > --- a/gcc/testsuite/g++.dg/cpp0x/initlist92.C

> > > > +++ b/gcc/testsuite/g++.dg/cpp0x/initlist92.C

> > > > @@ -23,7 +23,7 @@ bool Test4(std::initializer_list<std::string>);

> > > >   int main ()

> > > >   {

> > > > -  ( Test1({"false"}) );    // { dg-error "narrowing" "" { target

> > > > c++2a } }

> > > > +  ( Test1({"false"}) );    // { dg-error "narrowing" }

> > > >     ( Test2({123}) );

> > > >     ( Test3({456}) );

> > > >     ( Test4({"false"}) );

> > > > 

> > > > base-commit: dbfba41e95d1d93b17e907b7f516b52ed3a3c415

> > > > 

> > > 

> > 

> 

> Marek

> 


Marek
Jeff Law March 2, 2020, 4:25 p.m. | #6
On Mon, 2020-03-02 at 10:45 -0500, Marek Polacek wrote:
> Ping: Jeff, see the question below.  Thanks.

Sorry, totally missed the question.  I'm guessing you want me to run it through
the Fedora build tester?

jeff
>
Marek Polacek March 2, 2020, 4:29 p.m. | #7
On Mon, Mar 02, 2020 at 09:25:35AM -0700, Jeff Law wrote:
> On Mon, 2020-03-02 at 10:45 -0500, Marek Polacek wrote:

> > Ping: Jeff, see the question below.  Thanks.

> Sorry, totally missed the question.  I'm guessing you want me to run it through

> the Fedora build tester?


Yeah -- we want to allow a more strict narrow checking even in pre-C+20 modes,
and we're wondering about the fallout.  So re-compiling Fedora packages would
be useful.

Marek
Jeff Law March 2, 2020, 4:38 p.m. | #8
On Mon, 2020-03-02 at 11:29 -0500, Marek Polacek wrote:
> On Mon, Mar 02, 2020 at 09:25:35AM -0700, Jeff Law wrote:

> > On Mon, 2020-03-02 at 10:45 -0500, Marek Polacek wrote:

> > > Ping: Jeff, see the question below.  Thanks.

> > Sorry, totally missed the question.  I'm guessing you want me to run it

> > through

> > the Fedora build tester?

> 

> Yeah -- we want to allow a more strict narrow checking even in pre-C+20

> modes, and we're wondering about the fallout.  So re-compiling Fedora

> packages would be useful.

Is the error easy to identify from a diagnostic?  If so that would avoid having
to get fresh baselines.  I can just apply the patch, build gcc, then spin up a
single build of everything.

Jeff
Jason Merrill March 2, 2020, 4:59 p.m. | #9
On 3/2/20 11:38 AM, Jeff Law wrote:
> On Mon, 2020-03-02 at 11:29 -0500, Marek Polacek wrote:

>> On Mon, Mar 02, 2020 at 09:25:35AM -0700, Jeff Law wrote:

>>> On Mon, 2020-03-02 at 10:45 -0500, Marek Polacek wrote:

>>>> Ping: Jeff, see the question below.  Thanks.

>>> Sorry, totally missed the question.  I'm guessing you want me to run it

>>> through

>>> the Fedora build tester?

>>

>> Yeah -- we want to allow a more strict narrow checking even in pre-C+20

>> modes, and we're wondering about the fallout.  So re-compiling Fedora

>> packages would be useful.

> Is the error easy to identify from a diagnostic?  If so that would avoid having

> to get fresh baselines.  I can just apply the patch, build gcc, then spin up a

> single build of everything.


Not always; sometimes making something ill-formed just changes overload 
resolution

On further thought, it seems unnecessary to make this change at this 
point in the release cycle.  Let's defer this to GCC 11.

Jason
Marek Polacek March 2, 2020, 5:06 p.m. | #10
On Mon, Mar 02, 2020 at 11:59:12AM -0500, Jason Merrill wrote:
> On 3/2/20 11:38 AM, Jeff Law wrote:

> > On Mon, 2020-03-02 at 11:29 -0500, Marek Polacek wrote:

> > > On Mon, Mar 02, 2020 at 09:25:35AM -0700, Jeff Law wrote:

> > > > On Mon, 2020-03-02 at 10:45 -0500, Marek Polacek wrote:

> > > > > Ping: Jeff, see the question below.  Thanks.

> > > > Sorry, totally missed the question.  I'm guessing you want me to run it

> > > > through

> > > > the Fedora build tester?

> > > 

> > > Yeah -- we want to allow a more strict narrow checking even in pre-C+20

> > > modes, and we're wondering about the fallout.  So re-compiling Fedora

> > > packages would be useful.

> > Is the error easy to identify from a diagnostic?  If so that would avoid having

> > to get fresh baselines.  I can just apply the patch, build gcc, then spin up a

> > single build of everything.

> 

> Not always; sometimes making something ill-formed just changes overload

> resolution

> 

> On further thought, it seems unnecessary to make this change at this point

> in the release cycle.  Let's defer this to GCC 11.


Ack, will defer then.

Marek

Patch

diff --git a/gcc/cp/typeck2.c b/gcc/cp/typeck2.c
index 48920894b3b..68bc2e5c170 100644
--- a/gcc/cp/typeck2.c
+++ b/gcc/cp/typeck2.c
@@ -1036,9 +1036,10 @@  check_narrowing (tree type, tree init, tsubst_flags_t complain,
     }
   else if (TREE_CODE (type) == BOOLEAN_TYPE
 	   && (TYPE_PTR_P (ftype) || TYPE_PTRMEM_P (ftype)))
-    /* This hasn't actually made it into C++20 yet, but let's add it now to get
-       an idea of the impact.  */
-    ok = (cxx_dialect < cxx2a);
+    /* C++20 P1957R2: converting from a pointer type or a pointer-to-member
+       type to bool should be considered narrowing.  This is a DR so is not
+       limited to C++20 only.  */
+    ok = false;
 
   bool almost_ok = ok;
   if (!ok && !CONSTANT_CLASS_P (init) && (complain & tf_warning_or_error))
diff --git a/gcc/testsuite/g++.dg/cpp0x/initlist92.C b/gcc/testsuite/g++.dg/cpp0x/initlist92.C
index 319264ae274..213b192d441 100644
--- a/gcc/testsuite/g++.dg/cpp0x/initlist92.C
+++ b/gcc/testsuite/g++.dg/cpp0x/initlist92.C
@@ -23,7 +23,7 @@  bool Test4(std::initializer_list<std::string>);
 
 int main () 
 {
-  ( Test1({"false"}) );	// { dg-error "narrowing" "" { target c++2a } }
+  ( Test1({"false"}) );	// { dg-error "narrowing" }
   ( Test2({123}) );
   ( Test3({456}) );
   ( Test4({"false"}) );