[v2] C++ concepts: fix ICE with requires on dtors (PR c++/89036)

Message ID 1548446549-54085-1-git-send-email-dmalcolm@redhat.com
State Superseded
Headers show
Series
  • [v2] C++ concepts: fix ICE with requires on dtors (PR c++/89036)
Related show

Commit Message

David Malcolm Jan. 25, 2019, 8:02 p.m.
On Fri, 2019-01-25 at 08:59 -0800, Nathan Sidwell wrote:
> On 1/25/19 8:48 AM, David Malcolm wrote:

> > PR c++/89036 reports an ICE due to this assertion failing

> > 

> > 1136	  /* A class should never have more than one

> > destructor.  */

> > 1137	  gcc_assert (!current_fns || via_using ||

> > !DECL_DESTRUCTOR_P (method));

> > 

> > on this template with a pair of dtors, with

> > mutually exclusive "requires" clauses:

> > 

> > template<typename T>

> > struct Y {

> >      ~Y() requires(true) = default;

> >      ~Y() requires(false) {}

> > };

> > 

> > (is this valid?  my knowledge of this part of C++ is fairly hazy)

> 

> Yes. A more sensible example would have 'true' and 'false' replaced

> by 

> something determined from the template parms.

> 

> > 

> > Nathan introduced this assertion as part of:

> > 

> >    ca9219bf18c68a001d62ecb981bc9176b0feaf12 (aka r251340):

> >      2017-08-24  Nathan Sidwell  <nathan@acm.org>

> >         Conversion operators kept on single overload set

> 

> I'd just drop the assert at this point.

> 

> nathan


Thanks.  Here's a version of the patch which drops the assertion.

Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.

OK for trunk?

gcc/cp/ChangeLog:
	PR c++/89036
	* class.c (add_method): Drop destructor assertion.

gcc/testsuite/ChangeLog:
	PR c++/89036
	* g++.dg/concepts/pr89036.C: New test.
---
 gcc/cp/class.c                          | 3 ---
 gcc/testsuite/g++.dg/concepts/pr89036.C | 8 ++++++++
 2 files changed, 8 insertions(+), 3 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/concepts/pr89036.C

-- 
1.8.5.3

Comments

David Malcolm Feb. 8, 2019, 5:03 p.m. | #1
Ping

On Fri, 2019-01-25 at 15:02 -0500, David Malcolm wrote:
> On Fri, 2019-01-25 at 08:59 -0800, Nathan Sidwell wrote:

> > On 1/25/19 8:48 AM, David Malcolm wrote:

> > > PR c++/89036 reports an ICE due to this assertion failing

> > > 

> > > 1136	  /* A class should never have more than one

> > > destructor.  */

> > > 1137	  gcc_assert (!current_fns || via_using ||

> > > !DECL_DESTRUCTOR_P (method));

> > > 

> > > on this template with a pair of dtors, with

> > > mutually exclusive "requires" clauses:

> > > 

> > > template<typename T>

> > > struct Y {

> > >      ~Y() requires(true) = default;

> > >      ~Y() requires(false) {}

> > > };

> > > 

> > > (is this valid?  my knowledge of this part of C++ is fairly hazy)

> > 

> > Yes. A more sensible example would have 'true' and 'false' replaced

> > by 

> > something determined from the template parms.

> > 

> > > 

> > > Nathan introduced this assertion as part of:

> > > 

> > >    ca9219bf18c68a001d62ecb981bc9176b0feaf12 (aka r251340):

> > >      2017-08-24  Nathan Sidwell  <nathan@acm.org>

> > >         Conversion operators kept on single overload set

> > 

> > I'd just drop the assert at this point.

> > 

> > nathan

> 

> Thanks.  Here's a version of the patch which drops the assertion.

> 

> Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.

> 

> OK for trunk?

> 

> gcc/cp/ChangeLog:

> 	PR c++/89036

> 	* class.c (add_method): Drop destructor assertion.

> 

> gcc/testsuite/ChangeLog:

> 	PR c++/89036

> 	* g++.dg/concepts/pr89036.C: New test.

> ---

>  gcc/cp/class.c                          | 3 ---

>  gcc/testsuite/g++.dg/concepts/pr89036.C | 8 ++++++++

>  2 files changed, 8 insertions(+), 3 deletions(-)

>  create mode 100644 gcc/testsuite/g++.dg/concepts/pr89036.C

> 

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

> index e8773c2..6e9dac9 100644

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

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

> @@ -1133,9 +1133,6 @@ add_method (tree type, tree method, bool

> via_using)

>  	}

>      }

>  

> -  /* A class should never have more than one destructor.  */

> -  gcc_assert (!current_fns || via_using || !DECL_DESTRUCTOR_P

> (method));

> -

>    current_fns = ovl_insert (method, current_fns, via_using);

>  

>    if (!COMPLETE_TYPE_P (type) && !DECL_CONV_FN_P (method)

> diff --git a/gcc/testsuite/g++.dg/concepts/pr89036.C

> b/gcc/testsuite/g++.dg/concepts/pr89036.C

> new file mode 100644

> index 0000000..f83ef8b

> --- /dev/null

> +++ b/gcc/testsuite/g++.dg/concepts/pr89036.C

> @@ -0,0 +1,8 @@

> +// { dg-do compile { target c++11 } }

> +// { dg-options "-fconcepts" }

> +

> +template<typename T>

> +struct Y {

> +  ~Y() requires(true) = default;

> +  ~Y() requires(false) {}

> +};
David Malcolm Feb. 13, 2019, 2:36 p.m. | #2
Ping

On Fri, 2019-02-08 at 12:03 -0500, David Malcolm wrote:
> Ping

> 

> On Fri, 2019-01-25 at 15:02 -0500, David Malcolm wrote:

> > On Fri, 2019-01-25 at 08:59 -0800, Nathan Sidwell wrote:

> > > On 1/25/19 8:48 AM, David Malcolm wrote:

> > > > PR c++/89036 reports an ICE due to this assertion failing

> > > > 

> > > > 1136	  /* A class should never have more than one

> > > > destructor.  */

> > > > 1137	  gcc_assert (!current_fns || via_using ||

> > > > !DECL_DESTRUCTOR_P (method));

> > > > 

> > > > on this template with a pair of dtors, with

> > > > mutually exclusive "requires" clauses:

> > > > 

> > > > template<typename T>

> > > > struct Y {

> > > >      ~Y() requires(true) = default;

> > > >      ~Y() requires(false) {}

> > > > };

> > > > 

> > > > (is this valid?  my knowledge of this part of C++ is fairly

> > > > hazy)

> > > 

> > > Yes. A more sensible example would have 'true' and 'false'

> > > replaced

> > > by 

> > > something determined from the template parms.

> > > 

> > > > 

> > > > Nathan introduced this assertion as part of:

> > > > 

> > > >    ca9219bf18c68a001d62ecb981bc9176b0feaf12 (aka r251340):

> > > >      2017-08-24  Nathan Sidwell  <nathan@acm.org>

> > > >         Conversion operators kept on single overload set

> > > 

> > > I'd just drop the assert at this point.

> > > 

> > > nathan

> > 

> > Thanks.  Here's a version of the patch which drops the assertion.

> > 

> > Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.

> > 

> > OK for trunk?

> > 

> > gcc/cp/ChangeLog:

> > 	PR c++/89036

> > 	* class.c (add_method): Drop destructor assertion.

> > 

> > gcc/testsuite/ChangeLog:

> > 	PR c++/89036

> > 	* g++.dg/concepts/pr89036.C: New test.

> > ---

> >  gcc/cp/class.c                          | 3 ---

> >  gcc/testsuite/g++.dg/concepts/pr89036.C | 8 ++++++++

> >  2 files changed, 8 insertions(+), 3 deletions(-)

> >  create mode 100644 gcc/testsuite/g++.dg/concepts/pr89036.C

> > 

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

> > index e8773c2..6e9dac9 100644

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

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

> > @@ -1133,9 +1133,6 @@ add_method (tree type, tree method, bool

> > via_using)

> >  	}

> >      }

> >  

> > -  /* A class should never have more than one destructor.  */

> > -  gcc_assert (!current_fns || via_using || !DECL_DESTRUCTOR_P

> > (method));

> > -

> >    current_fns = ovl_insert (method, current_fns, via_using);

> >  

> >    if (!COMPLETE_TYPE_P (type) && !DECL_CONV_FN_P (method)

> > diff --git a/gcc/testsuite/g++.dg/concepts/pr89036.C

> > b/gcc/testsuite/g++.dg/concepts/pr89036.C

> > new file mode 100644

> > index 0000000..f83ef8b

> > --- /dev/null

> > +++ b/gcc/testsuite/g++.dg/concepts/pr89036.C

> > @@ -0,0 +1,8 @@

> > +// { dg-do compile { target c++11 } }

> > +// { dg-options "-fconcepts" }

> > +

> > +template<typename T>

> > +struct Y {

> > +  ~Y() requires(true) = default;

> > +  ~Y() requires(false) {}

> > +};
Nathan Sidwell Feb. 13, 2019, 2:39 p.m. | #3
On 2/13/19 9:36 AM, David Malcolm wrote:
> Ping


yes, sorry didn't realize you were waiting on me.


> On Fri, 2019-02-08 at 12:03 -0500, David Malcolm wrote:

>> Ping

>>

>> On Fri, 2019-01-25 at 15:02 -0500, David Malcolm wrote:

>>> On Fri, 2019-01-25 at 08:59 -0800, Nathan Sidwell wrote:

>>>> On 1/25/19 8:48 AM, David Malcolm wrote:

>>>>> PR c++/89036 reports an ICE due to this assertion failing

>>>>>

>>>>> 1136	  /* A class should never have more than one

>>>>> destructor.  */

>>>>> 1137	  gcc_assert (!current_fns || via_using ||

>>>>> !DECL_DESTRUCTOR_P (method));

>>>>>

>>>>> on this template with a pair of dtors, with

>>>>> mutually exclusive "requires" clauses:

>>>>>

>>>>> template<typename T>

>>>>> struct Y {

>>>>>       ~Y() requires(true) = default;

>>>>>       ~Y() requires(false) {}

>>>>> };

>>>>>

>>>>> (is this valid?  my knowledge of this part of C++ is fairly

>>>>> hazy)

>>>>

>>>> Yes. A more sensible example would have 'true' and 'false'

>>>> replaced

>>>> by

>>>> something determined from the template parms.

>>>>

>>>>>

>>>>> Nathan introduced this assertion as part of:

>>>>>

>>>>>     ca9219bf18c68a001d62ecb981bc9176b0feaf12 (aka r251340):

>>>>>       2017-08-24  Nathan Sidwell  <nathan@acm.org>

>>>>>          Conversion operators kept on single overload set

>>>>

>>>> I'd just drop the assert at this point.

>>>>

>>>> nathan

>>>

>>> Thanks.  Here's a version of the patch which drops the assertion.

>>>

>>> Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.

>>>

>>> OK for trunk?

>>>

>>> gcc/cp/ChangeLog:

>>> 	PR c++/89036

>>> 	* class.c (add_method): Drop destructor assertion.

>>>

>>> gcc/testsuite/ChangeLog:

>>> 	PR c++/89036

>>> 	* g++.dg/concepts/pr89036.C: New test.

>>> ---

>>>   gcc/cp/class.c                          | 3 ---

>>>   gcc/testsuite/g++.dg/concepts/pr89036.C | 8 ++++++++

>>>   2 files changed, 8 insertions(+), 3 deletions(-)

>>>   create mode 100644 gcc/testsuite/g++.dg/concepts/pr89036.C

>>>

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

>>> index e8773c2..6e9dac9 100644

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

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

>>> @@ -1133,9 +1133,6 @@ add_method (tree type, tree method, bool

>>> via_using)

>>>   	}

>>>       }

>>>   

>>> -  /* A class should never have more than one destructor.  */

>>> -  gcc_assert (!current_fns || via_using || !DECL_DESTRUCTOR_P

>>> (method));

>>> -

>>>     current_fns = ovl_insert (method, current_fns, via_using);

>>>   

>>>     if (!COMPLETE_TYPE_P (type) && !DECL_CONV_FN_P (method)

>>> diff --git a/gcc/testsuite/g++.dg/concepts/pr89036.C

>>> b/gcc/testsuite/g++.dg/concepts/pr89036.C

>>> new file mode 100644

>>> index 0000000..f83ef8b

>>> --- /dev/null

>>> +++ b/gcc/testsuite/g++.dg/concepts/pr89036.C

>>> @@ -0,0 +1,8 @@

>>> +// { dg-do compile { target c++11 } }

>>> +// { dg-options "-fconcepts" }

>>> +

>>> +template<typename T>

>>> +struct Y {

>>> +  ~Y() requires(true) = default;

>>> +  ~Y() requires(false) {}

>>> +};



-- 
Nathan Sidwell

Patch

diff --git a/gcc/cp/class.c b/gcc/cp/class.c
index e8773c2..6e9dac9 100644
--- a/gcc/cp/class.c
+++ b/gcc/cp/class.c
@@ -1133,9 +1133,6 @@  add_method (tree type, tree method, bool via_using)
 	}
     }
 
-  /* A class should never have more than one destructor.  */
-  gcc_assert (!current_fns || via_using || !DECL_DESTRUCTOR_P (method));
-
   current_fns = ovl_insert (method, current_fns, via_using);
 
   if (!COMPLETE_TYPE_P (type) && !DECL_CONV_FN_P (method)
diff --git a/gcc/testsuite/g++.dg/concepts/pr89036.C b/gcc/testsuite/g++.dg/concepts/pr89036.C
new file mode 100644
index 0000000..f83ef8b
--- /dev/null
+++ b/gcc/testsuite/g++.dg/concepts/pr89036.C
@@ -0,0 +1,8 @@ 
+// { dg-do compile { target c++11 } }
+// { dg-options "-fconcepts" }
+
+template<typename T>
+struct Y {
+  ~Y() requires(true) = default;
+  ~Y() requires(false) {}
+};