[C++] PR 84644 ("internal compiler error: in warn_misplaced_attr_for_class_type, at cp/decl.c:4718")

Message ID ae3577b1-17ba-5b16-f767-0172da0a3d25@oracle.com
State New
Headers show
Series
  • [C++] PR 84644 ("internal compiler error: in warn_misplaced_attr_for_class_type, at cp/decl.c:4718")
Related show

Commit Message

Paolo Carlini Oct. 15, 2018, 4:45 p.m.
Hi,

here we ICE when, at the end of check_tag_decl we pass a DECLTYPE_TYPE 
to warn_misplaced_attr_for_class_type. I think the right fix is 
rejecting earlier a decltype with no declarator as a declaration that 
doesn't declare anything (note: all the compilers I have at hand agree). 
Tested x86_64-linux.

Thanks, Paolo.

////////////////////
/cp
2018-10-15  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/84644
	* decl.c (check_tag_decl): A decltype with no declarator
	doesn't declare anything.

/testsuite
2018-10-15  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/84644
	* g++.dg/cpp0x/decltype68.C: New.
	* g++.dg/cpp0x/decltype-33838.C: Adjust.

Comments

Jason Merrill Oct. 24, 2018, 8:41 p.m. | #1
On 10/15/18 12:45 PM, Paolo Carlini wrote:
>         && ((TREE_CODE (declspecs->type) != TYPENAME_TYPE

> +	   && TREE_CODE (declspecs->type) != DECLTYPE_TYPE

>   	   && MAYBE_CLASS_TYPE_P (declspecs->type))


I would think that the MAYBE_CLASS_TYPE_P here should be CLASS_TYPE_P, 
and then we can remove the TYPENAME_TYPE check.  Or do we want to allow 
template type parameters for some reason?

Jason
Paolo Carlini Oct. 26, 2018, 8:50 a.m. | #2
Hi,

On 24/10/18 22:41, Jason Merrill wrote:
> On 10/15/18 12:45 PM, Paolo Carlini wrote:

>>         && ((TREE_CODE (declspecs->type) != TYPENAME_TYPE

>> +       && TREE_CODE (declspecs->type) != DECLTYPE_TYPE

>>          && MAYBE_CLASS_TYPE_P (declspecs->type))

>

> I would think that the MAYBE_CLASS_TYPE_P here should be CLASS_TYPE_P, 

> and then we can remove the TYPENAME_TYPE check.  Or do we want to 

> allow template type parameters for some reason?


Indeed, it would be nice to just use OVERLOAD_TYPE_P. However it seems 
we at least want to let through TEMPLATE_TYPE_PARMs representing 'auto' 
- otherwise Dodji's check a few lines below which fixed c++/51473 
doesn't work anymore - and also BOUND_TEMPLATE_TEMPLATE_PARM, otherwise 
we regress on template/spec32.C and template/ttp22.C because we don't 
diagnose the shadowing anymore. Thus, I would say either we keep on 
using MAYBE_CLASS_TYPE_P or we pick what we need, possibly we add a comment?

Thanks, Paolo.
Jason Merrill Oct. 26, 2018, 3:18 p.m. | #3
On Fri, Oct 26, 2018 at 4:52 AM Paolo Carlini <paolo.carlini@oracle.com> wrote:
> On 24/10/18 22:41, Jason Merrill wrote:

> > On 10/15/18 12:45 PM, Paolo Carlini wrote:

> >>         && ((TREE_CODE (declspecs->type) != TYPENAME_TYPE

> >> +       && TREE_CODE (declspecs->type) != DECLTYPE_TYPE

> >>          && MAYBE_CLASS_TYPE_P (declspecs->type))

> >

> > I would think that the MAYBE_CLASS_TYPE_P here should be CLASS_TYPE_P,

> > and then we can remove the TYPENAME_TYPE check.  Or do we want to

> > allow template type parameters for some reason?

>

> Indeed, it would be nice to just use OVERLOAD_TYPE_P. However it seems

> we at least want to let through TEMPLATE_TYPE_PARMs representing 'auto'

> - otherwise Dodji's check a few lines below which fixed c++/51473

> doesn't work anymore - and also BOUND_TEMPLATE_TEMPLATE_PARM, otherwise

> we regress on template/spec32.C and template/ttp22.C because we don't

> diagnose the shadowing anymore. Thus, I would say either we keep on

> using MAYBE_CLASS_TYPE_P or we pick what we need, possibly we add a comment?


Aha.  I guess the answer is not to restrict that test any more, but
instead to fix the code further down so it gives a proper diagnostic
rather than call warn_misplaced_attr_for_class_type.

Jason
Paolo Carlini Oct. 26, 2018, 6:02 p.m. | #4
Hi,

On 26/10/18 17:18, Jason Merrill wrote:
> On Fri, Oct 26, 2018 at 4:52 AM Paolo Carlini <paolo.carlini@oracle.com> wrote:

>> On 24/10/18 22:41, Jason Merrill wrote:

>>> On 10/15/18 12:45 PM, Paolo Carlini wrote:

>>>>          && ((TREE_CODE (declspecs->type) != TYPENAME_TYPE

>>>> +       && TREE_CODE (declspecs->type) != DECLTYPE_TYPE

>>>>           && MAYBE_CLASS_TYPE_P (declspecs->type))

>>> I would think that the MAYBE_CLASS_TYPE_P here should be CLASS_TYPE_P,

>>> and then we can remove the TYPENAME_TYPE check.  Or do we want to

>>> allow template type parameters for some reason?

>> Indeed, it would be nice to just use OVERLOAD_TYPE_P. However it seems

>> we at least want to let through TEMPLATE_TYPE_PARMs representing 'auto'

>> - otherwise Dodji's check a few lines below which fixed c++/51473

>> doesn't work anymore - and also BOUND_TEMPLATE_TEMPLATE_PARM, otherwise

>> we regress on template/spec32.C and template/ttp22.C because we don't

>> diagnose the shadowing anymore. Thus, I would say either we keep on

>> using MAYBE_CLASS_TYPE_P or we pick what we need, possibly we add a comment?

> Aha.  I guess the answer is not to restrict that test any more, but

> instead to fix the code further down so it gives a proper diagnostic

> rather than call warn_misplaced_attr_for_class_type.


I see. Thus something like the below? It passes testing on x86_64-linux.

Thanks! Paolo.

/////////////
Index: cp/decl.c
===================================================================
--- cp/decl.c	(revision 265510)
+++ cp/decl.c	(working copy)
@@ -4798,9 +4798,10 @@ check_tag_decl (cp_decl_specifier_seq *declspecs,
     declared_type = declspecs->type;
   else if (declspecs->type == error_mark_node)
     error_p = true;
-  if (declared_type == NULL_TREE && ! saw_friend && !error_p)
+  if ((!declared_type || TREE_CODE (declared_type) == DECLTYPE_TYPE)
+      && ! saw_friend && !error_p)
     permerror (input_location, "declaration does not declare anything");
-  else if (declared_type != NULL_TREE && type_uses_auto (declared_type))
+  else if (declared_type && type_uses_auto (declared_type))
     {
       error_at (declspecs->locations[ds_type_spec],
 		"%<auto%> can only be specified for variables "
@@ -4884,7 +4885,8 @@ check_tag_decl (cp_decl_specifier_seq *declspecs,
 		  "%<constexpr%> cannot be used for type declarations");
     }
 
-  if (declspecs->attributes && warn_attributes && declared_type)
+  if (declspecs->attributes && warn_attributes && declared_type
+      && TREE_CODE (declared_type) != DECLTYPE_TYPE)
     {
       location_t loc;
       if (!CLASS_TYPE_P (declared_type)
Index: testsuite/g++.dg/cpp0x/decltype-33838.C
===================================================================
--- testsuite/g++.dg/cpp0x/decltype-33838.C	(revision 265510)
+++ testsuite/g++.dg/cpp0x/decltype-33838.C	(working copy)
@@ -2,5 +2,5 @@
 // PR c++/33838
 template<typename T> struct A
 {
-  __decltype (T* foo()); // { dg-error "expected|no arguments|accept" }
+  __decltype (T* foo()); // { dg-error "expected|no arguments|declaration" }
 };
Index: testsuite/g++.dg/cpp0x/decltype68.C
===================================================================
--- testsuite/g++.dg/cpp0x/decltype68.C	(nonexistent)
+++ testsuite/g++.dg/cpp0x/decltype68.C	(working copy)
@@ -0,0 +1,7 @@
+// PR c++/84644
+// { dg-do compile { target c++11 } }
+
+template<int a>
+struct b {
+  decltype(a) __attribute__((break));  // { dg-error "declaration does not declare anything" }
+};
Jason Merrill Oct. 30, 2018, 8:37 p.m. | #5
On 10/26/18 2:02 PM, Paolo Carlini wrote:
> On 26/10/18 17:18, Jason Merrill wrote:

>> On Fri, Oct 26, 2018 at 4:52 AM Paolo Carlini 

>> <paolo.carlini@oracle.com> wrote:

>>> On 24/10/18 22:41, Jason Merrill wrote:

>>>> On 10/15/18 12:45 PM, Paolo Carlini wrote:

>>>>>          && ((TREE_CODE (declspecs->type) != TYPENAME_TYPE

>>>>> +       && TREE_CODE (declspecs->type) != DECLTYPE_TYPE

>>>>>           && MAYBE_CLASS_TYPE_P (declspecs->type))

>>>> I would think that the MAYBE_CLASS_TYPE_P here should be CLASS_TYPE_P,

>>>> and then we can remove the TYPENAME_TYPE check.  Or do we want to

>>>> allow template type parameters for some reason?

>>> Indeed, it would be nice to just use OVERLOAD_TYPE_P. However it seems

>>> we at least want to let through TEMPLATE_TYPE_PARMs representing 'auto'

>>> - otherwise Dodji's check a few lines below which fixed c++/51473

>>> doesn't work anymore - and also BOUND_TEMPLATE_TEMPLATE_PARM, otherwise

>>> we regress on template/spec32.C and template/ttp22.C because we don't

>>> diagnose the shadowing anymore. Thus, I would say either we keep on

>>> using MAYBE_CLASS_TYPE_P or we pick what we need, possibly we add a 

>>> comment?

>> Aha.  I guess the answer is not to restrict that test any more, but

>> instead to fix the code further down so it gives a proper diagnostic

>> rather than call warn_misplaced_attr_for_class_type.

> 

> I see. Thus something like the below? It passes testing on x86_64-linux.


> +  if ((!declared_type || TREE_CODE (declared_type) == DECLTYPE_TYPE)

> +      && ! saw_friend && !error_p)

>      permerror (input_location, "declaration does not declare anything");


I see no reason to make this specific to decltype.  Maybe move this 
diagnostic into the final 'else' block with the other declspec 
diagnostics and not look at declared_type at all?

> +  if (declspecs->attributes && warn_attributes && declared_type

> +      && TREE_CODE (declared_type) != DECLTYPE_TYPE)


I think we do want to give a diagnostic about useless attributes, not 
skip it.

Jason
Paolo Carlini Oct. 31, 2018, 1:22 a.m. | #6
Hi,

On 30/10/18 21:37, Jason Merrill wrote:
> On 10/26/18 2:02 PM, Paolo Carlini wrote:

>> On 26/10/18 17:18, Jason Merrill wrote:

>>> On Fri, Oct 26, 2018 at 4:52 AM Paolo Carlini 

>>> <paolo.carlini@oracle.com> wrote:

>>>> On 24/10/18 22:41, Jason Merrill wrote:

>>>>> On 10/15/18 12:45 PM, Paolo Carlini wrote:

>>>>>>          && ((TREE_CODE (declspecs->type) != TYPENAME_TYPE

>>>>>> +       && TREE_CODE (declspecs->type) != DECLTYPE_TYPE

>>>>>>           && MAYBE_CLASS_TYPE_P (declspecs->type))

>>>>> I would think that the MAYBE_CLASS_TYPE_P here should be 

>>>>> CLASS_TYPE_P,

>>>>> and then we can remove the TYPENAME_TYPE check.  Or do we want to

>>>>> allow template type parameters for some reason?

>>>> Indeed, it would be nice to just use OVERLOAD_TYPE_P. However it seems

>>>> we at least want to let through TEMPLATE_TYPE_PARMs representing 

>>>> 'auto'

>>>> - otherwise Dodji's check a few lines below which fixed c++/51473

>>>> doesn't work anymore - and also BOUND_TEMPLATE_TEMPLATE_PARM, 

>>>> otherwise

>>>> we regress on template/spec32.C and template/ttp22.C because we don't

>>>> diagnose the shadowing anymore. Thus, I would say either we keep on

>>>> using MAYBE_CLASS_TYPE_P or we pick what we need, possibly we add a 

>>>> comment?

>>> Aha.  I guess the answer is not to restrict that test any more, but

>>> instead to fix the code further down so it gives a proper diagnostic

>>> rather than call warn_misplaced_attr_for_class_type.

>>

>> I see. Thus something like the below? It passes testing on x86_64-linux.

>

>> +  if ((!declared_type || TREE_CODE (declared_type) == DECLTYPE_TYPE)

>> +      && ! saw_friend && !error_p)

>>      permerror (input_location, "declaration does not declare 

>> anything");

>

> I see no reason to make this specific to decltype.  Maybe move this 

> diagnostic into the final 'else' block with the other declspec 

> diagnostics and not look at declared_type at all?


I'm not sure to fully understand: if we do that we still want to at 
least minimally check that declared_type is null, like we already do, 
and then we simply accept the new testcase. Is that Ok? Because, as I 
probably mentioned at some point, all the other compilers I have at hand 
issue a "does not declare anything" diagnostic, and we likewise do that 
for the legacy __typeof. Not looking into declared_type *at all* doesn't 
work with plain class types and enums, of course. Or you meant something 
entirely different??

>> +  if (declspecs->attributes && warn_attributes && declared_type

>> +      && TREE_CODE (declared_type) != DECLTYPE_TYPE)

>

> I think we do want to give a diagnostic about useless attributes, not 

> skip it.


Agreed. FWIW the attached tests fine.

Thanks, Paolo.

///////////////////
Index: decl.c
===================================================================
--- decl.c	(revision 265636)
+++ decl.c	(working copy)
@@ -4798,9 +4798,7 @@ check_tag_decl (cp_decl_specifier_seq *declspecs,
     declared_type = declspecs->type;
   else if (declspecs->type == error_mark_node)
     error_p = true;
-  if (declared_type == NULL_TREE && ! saw_friend && !error_p)
-    permerror (input_location, "declaration does not declare anything");
-  else if (declared_type != NULL_TREE && type_uses_auto (declared_type))
+  if (declared_type && type_uses_auto (declared_type))
     {
       error_at (declspecs->locations[ds_type_spec],
 		"%<auto%> can only be specified for variables "
@@ -4842,7 +4840,9 @@ check_tag_decl (cp_decl_specifier_seq *declspecs,
 
   else
     {
-      if (decl_spec_seq_has_spec_p (declspecs, ds_inline))
+      if (!declared_type && ! saw_friend && !error_p)
+	permerror (input_location, "declaration does not declare anything");
+      else if (decl_spec_seq_has_spec_p (declspecs, ds_inline))
 	error_at (declspecs->locations[ds_inline],
 		  "%<inline%> can only be specified for functions");
       else if (decl_spec_seq_has_spec_p (declspecs, ds_virtual))
@@ -4909,7 +4909,7 @@ check_tag_decl (cp_decl_specifier_seq *declspecs,
 		    "no attribute can be applied to "
 		    "an explicit instantiation");
 	}
-      else
+      else if (TREE_CODE (declared_type) != DECLTYPE_TYPE)
 	warn_misplaced_attr_for_class_type (loc, declared_type);
     }
Jason Merrill Dec. 13, 2018, 9:03 p.m. | #7
On 10/30/18 9:22 PM, Paolo Carlini wrote:
> Hi,

> 

> On 30/10/18 21:37, Jason Merrill wrote:

>> On 10/26/18 2:02 PM, Paolo Carlini wrote:

>>> On 26/10/18 17:18, Jason Merrill wrote:

>>>> On Fri, Oct 26, 2018 at 4:52 AM Paolo Carlini 

>>>> <paolo.carlini@oracle.com> wrote:

>>>>> On 24/10/18 22:41, Jason Merrill wrote:

>>>>>> On 10/15/18 12:45 PM, Paolo Carlini wrote:

>>>>>>>          && ((TREE_CODE (declspecs->type) != TYPENAME_TYPE

>>>>>>> +       && TREE_CODE (declspecs->type) != DECLTYPE_TYPE

>>>>>>>           && MAYBE_CLASS_TYPE_P (declspecs->type))

>>>>>> I would think that the MAYBE_CLASS_TYPE_P here should be 

>>>>>> CLASS_TYPE_P,

>>>>>> and then we can remove the TYPENAME_TYPE check.  Or do we want to

>>>>>> allow template type parameters for some reason?

>>>>> Indeed, it would be nice to just use OVERLOAD_TYPE_P. However it seems

>>>>> we at least want to let through TEMPLATE_TYPE_PARMs representing 

>>>>> 'auto'

>>>>> - otherwise Dodji's check a few lines below which fixed c++/51473

>>>>> doesn't work anymore - and also BOUND_TEMPLATE_TEMPLATE_PARM, 

>>>>> otherwise

>>>>> we regress on template/spec32.C and template/ttp22.C because we don't

>>>>> diagnose the shadowing anymore. Thus, I would say either we keep on

>>>>> using MAYBE_CLASS_TYPE_P or we pick what we need, possibly we add a 

>>>>> comment?

>>>> Aha.  I guess the answer is not to restrict that test any more, but

>>>> instead to fix the code further down so it gives a proper diagnostic

>>>> rather than call warn_misplaced_attr_for_class_type.

>>>

>>> I see. Thus something like the below? It passes testing on x86_64-linux.

>>

>>> +  if ((!declared_type || TREE_CODE (declared_type) == DECLTYPE_TYPE)

>>> +      && ! saw_friend && !error_p)

>>>      permerror (input_location, "declaration does not declare 

>>> anything");

>>

>> I see no reason to make this specific to decltype.  Maybe move this 

>> diagnostic into the final 'else' block with the other declspec 

>> diagnostics and not look at declared_type at all?

> 

> I'm not sure to fully understand: if we do that we still want to at 

> least minimally check that declared_type is null, like we already do, 

> and then we simply accept the new testcase. Is that Ok? Because, as I 

> probably mentioned at some point, all the other compilers I have at hand 

> issue a "does not declare anything" diagnostic, and we likewise do that 

> for the legacy __typeof. Not looking into declared_type *at all* doesn't 

> work with plain class types and enums, of course. Or you meant something 

> entirely different??

> 

>>> +  if (declspecs->attributes && warn_attributes && declared_type

>>> +      && TREE_CODE (declared_type) != DECLTYPE_TYPE)

>>

>> I think we do want to give a diagnostic about useless attributes, not 

>> skip it.

> 

> Agreed. FWIW the attached tests fine.


The problem here is that the code toward the bottom expects 
"declared_type" to be the tagged type declared by a declaration with no 
declarator, and in this testcase it's ending up as a DECLTYPE_TYPE.

I think once we've checked for 'auto' we don't want declared_type to be 
anything that isn't OVERLOAD_TYPE_P.  We can arrange that either by 
checking for 'auto' first and then changing the code that sets 
declared_type to use OVERLOAD_TYPE_P, or by clearing declared_type after 
checking for 'auto' if it isn't OVERLOAD_TYPE_P.

Jason
Paolo Carlini Dec. 14, 2018, 6:44 p.m. | #8
Hi,

On 13/12/18 22:03, Jason Merrill wrote:
> On 10/30/18 9:22 PM, Paolo Carlini wrote:

>> Hi,

>>

>> On 30/10/18 21:37, Jason Merrill wrote:

>>> On 10/26/18 2:02 PM, Paolo Carlini wrote:

>>>> On 26/10/18 17:18, Jason Merrill wrote:

>>>>> On Fri, Oct 26, 2018 at 4:52 AM Paolo Carlini 

>>>>> <paolo.carlini@oracle.com> wrote:

>>>>>> On 24/10/18 22:41, Jason Merrill wrote:

>>>>>>> On 10/15/18 12:45 PM, Paolo Carlini wrote:

>>>>>>>>          && ((TREE_CODE (declspecs->type) != TYPENAME_TYPE

>>>>>>>> +       && TREE_CODE (declspecs->type) != DECLTYPE_TYPE

>>>>>>>>           && MAYBE_CLASS_TYPE_P (declspecs->type))

>>>>>>> I would think that the MAYBE_CLASS_TYPE_P here should be 

>>>>>>> CLASS_TYPE_P,

>>>>>>> and then we can remove the TYPENAME_TYPE check.  Or do we want to

>>>>>>> allow template type parameters for some reason?

>>>>>> Indeed, it would be nice to just use OVERLOAD_TYPE_P. However it 

>>>>>> seems

>>>>>> we at least want to let through TEMPLATE_TYPE_PARMs representing 

>>>>>> 'auto'

>>>>>> - otherwise Dodji's check a few lines below which fixed c++/51473

>>>>>> doesn't work anymore - and also BOUND_TEMPLATE_TEMPLATE_PARM, 

>>>>>> otherwise

>>>>>> we regress on template/spec32.C and template/ttp22.C because we 

>>>>>> don't

>>>>>> diagnose the shadowing anymore. Thus, I would say either we keep on

>>>>>> using MAYBE_CLASS_TYPE_P or we pick what we need, possibly we add 

>>>>>> a comment?

>>>>> Aha.  I guess the answer is not to restrict that test any more, but

>>>>> instead to fix the code further down so it gives a proper diagnostic

>>>>> rather than call warn_misplaced_attr_for_class_type.

>>>>

>>>> I see. Thus something like the below? It passes testing on 

>>>> x86_64-linux.

>>>

>>>> +  if ((!declared_type || TREE_CODE (declared_type) == DECLTYPE_TYPE)

>>>> +      && ! saw_friend && !error_p)

>>>>      permerror (input_location, "declaration does not declare 

>>>> anything");

>>>

>>> I see no reason to make this specific to decltype.  Maybe move this 

>>> diagnostic into the final 'else' block with the other declspec 

>>> diagnostics and not look at declared_type at all?

>>

>> I'm not sure to fully understand: if we do that we still want to at 

>> least minimally check that declared_type is null, like we already do, 

>> and then we simply accept the new testcase. Is that Ok? Because, as I 

>> probably mentioned at some point, all the other compilers I have at 

>> hand issue a "does not declare anything" diagnostic, and we likewise 

>> do that for the legacy __typeof. Not looking into declared_type *at 

>> all* doesn't work with plain class types and enums, of course. Or you 

>> meant something entirely different??

>>

>>>> +  if (declspecs->attributes && warn_attributes && declared_type

>>>> +      && TREE_CODE (declared_type) != DECLTYPE_TYPE)

>>>

>>> I think we do want to give a diagnostic about useless attributes, 

>>> not skip it.

>>

>> Agreed. FWIW the attached tests fine.

>

> The problem here is that the code toward the bottom expects 

> "declared_type" to be the tagged type declared by a declaration with 

> no declarator, and in this testcase it's ending up as a DECLTYPE_TYPE.

>

> I think once we've checked for 'auto' we don't want declared_type to 

> be anything that isn't OVERLOAD_TYPE_P.  We can arrange that either by 

> checking for 'auto' first and then changing the code that sets 

> declared_type to use OVERLOAD_TYPE_P, or by clearing declared_type 

> after checking for 'auto' if it isn't OVERLOAD_TYPE_P.


Thanks. I'm slowly catching up on this issue... Any suggestion about 
BOUND_TEMPLATE_TEMPLATE_PARM? If we don't let through such tree nodes - 
which are MAYBE_CLASS_TYPE_P and aren't OVERLOAD_TYPE_P - we regress on 
template/spec32.C, we don't reject it anymore.,

Paolo.
Jason Merrill Dec. 14, 2018, 8:19 p.m. | #9
On 12/14/18 1:44 PM, Paolo Carlini wrote:
> Hi,

> 

> On 13/12/18 22:03, Jason Merrill wrote:

>> On 10/30/18 9:22 PM, Paolo Carlini wrote:

>>> Hi,

>>>

>>> On 30/10/18 21:37, Jason Merrill wrote:

>>>> On 10/26/18 2:02 PM, Paolo Carlini wrote:

>>>>> On 26/10/18 17:18, Jason Merrill wrote:

>>>>>> On Fri, Oct 26, 2018 at 4:52 AM Paolo Carlini 

>>>>>> <paolo.carlini@oracle.com> wrote:

>>>>>>> On 24/10/18 22:41, Jason Merrill wrote:

>>>>>>>> On 10/15/18 12:45 PM, Paolo Carlini wrote:

>>>>>>>>>          && ((TREE_CODE (declspecs->type) != TYPENAME_TYPE

>>>>>>>>> +       && TREE_CODE (declspecs->type) != DECLTYPE_TYPE

>>>>>>>>>           && MAYBE_CLASS_TYPE_P (declspecs->type))

>>>>>>>> I would think that the MAYBE_CLASS_TYPE_P here should be 

>>>>>>>> CLASS_TYPE_P,

>>>>>>>> and then we can remove the TYPENAME_TYPE check.  Or do we want to

>>>>>>>> allow template type parameters for some reason?

>>>>>>> Indeed, it would be nice to just use OVERLOAD_TYPE_P. However it 

>>>>>>> seems

>>>>>>> we at least want to let through TEMPLATE_TYPE_PARMs representing 

>>>>>>> 'auto'

>>>>>>> - otherwise Dodji's check a few lines below which fixed c++/51473

>>>>>>> doesn't work anymore - and also BOUND_TEMPLATE_TEMPLATE_PARM, 

>>>>>>> otherwise

>>>>>>> we regress on template/spec32.C and template/ttp22.C because we 

>>>>>>> don't

>>>>>>> diagnose the shadowing anymore. Thus, I would say either we keep on

>>>>>>> using MAYBE_CLASS_TYPE_P or we pick what we need, possibly we add 

>>>>>>> a comment?

>>>>>> Aha.  I guess the answer is not to restrict that test any more, but

>>>>>> instead to fix the code further down so it gives a proper diagnostic

>>>>>> rather than call warn_misplaced_attr_for_class_type.

>>>>>

>>>>> I see. Thus something like the below? It passes testing on 

>>>>> x86_64-linux.

>>>>

>>>>> +  if ((!declared_type || TREE_CODE (declared_type) == DECLTYPE_TYPE)

>>>>> +      && ! saw_friend && !error_p)

>>>>>      permerror (input_location, "declaration does not declare 

>>>>> anything");

>>>>

>>>> I see no reason to make this specific to decltype.  Maybe move this 

>>>> diagnostic into the final 'else' block with the other declspec 

>>>> diagnostics and not look at declared_type at all?

>>>

>>> I'm not sure to fully understand: if we do that we still want to at 

>>> least minimally check that declared_type is null, like we already do, 

>>> and then we simply accept the new testcase. Is that Ok? Because, as I 

>>> probably mentioned at some point, all the other compilers I have at 

>>> hand issue a "does not declare anything" diagnostic, and we likewise 

>>> do that for the legacy __typeof. Not looking into declared_type *at 

>>> all* doesn't work with plain class types and enums, of course. Or you 

>>> meant something entirely different??

>>>

>>>>> +  if (declspecs->attributes && warn_attributes && declared_type

>>>>> +      && TREE_CODE (declared_type) != DECLTYPE_TYPE)

>>>>

>>>> I think we do want to give a diagnostic about useless attributes, 

>>>> not skip it.

>>>

>>> Agreed. FWIW the attached tests fine.

>>

>> The problem here is that the code toward the bottom expects 

>> "declared_type" to be the tagged type declared by a declaration with 

>> no declarator, and in this testcase it's ending up as a DECLTYPE_TYPE.

>>

>> I think once we've checked for 'auto' we don't want declared_type to 

>> be anything that isn't OVERLOAD_TYPE_P.  We can arrange that either by 

>> checking for 'auto' first and then changing the code that sets 

>> declared_type to use OVERLOAD_TYPE_P, or by clearing declared_type 

>> after checking for 'auto' if it isn't OVERLOAD_TYPE_P.

> 

> Thanks. I'm slowly catching up on this issue... Any suggestion about 

> BOUND_TEMPLATE_TEMPLATE_PARM? If we don't let through such tree nodes - 

> which are MAYBE_CLASS_TYPE_P and aren't OVERLOAD_TYPE_P - we regress on 

> template/spec32.C, we don't reject it anymore.


If we clear declared_type for a BOUND_TEMPLATE_TEMPLATE_PARM, we should 
get the "does not declare anything" error.

Jason
Paolo Carlini Dec. 14, 2018, 9:33 p.m. | #10
Hi,

On 14/12/18 21:19, Jason Merrill wrote:
> On 12/14/18 1:44 PM, Paolo Carlini wrote:

>> Hi,

>>

>> On 13/12/18 22:03, Jason Merrill wrote:

>>> On 10/30/18 9:22 PM, Paolo Carlini wrote:

>>>> Hi,

>>>>

>>>> On 30/10/18 21:37, Jason Merrill wrote:

>>>>> On 10/26/18 2:02 PM, Paolo Carlini wrote:

>>>>>> On 26/10/18 17:18, Jason Merrill wrote:

>>>>>>> On Fri, Oct 26, 2018 at 4:52 AM Paolo Carlini 

>>>>>>> <paolo.carlini@oracle.com> wrote:

>>>>>>>> On 24/10/18 22:41, Jason Merrill wrote:

>>>>>>>>> On 10/15/18 12:45 PM, Paolo Carlini wrote:

>>>>>>>>>>          && ((TREE_CODE (declspecs->type) != TYPENAME_TYPE

>>>>>>>>>> +       && TREE_CODE (declspecs->type) != DECLTYPE_TYPE

>>>>>>>>>>           && MAYBE_CLASS_TYPE_P (declspecs->type))

>>>>>>>>> I would think that the MAYBE_CLASS_TYPE_P here should be 

>>>>>>>>> CLASS_TYPE_P,

>>>>>>>>> and then we can remove the TYPENAME_TYPE check. Or do we want to

>>>>>>>>> allow template type parameters for some reason?

>>>>>>>> Indeed, it would be nice to just use OVERLOAD_TYPE_P. However 

>>>>>>>> it seems

>>>>>>>> we at least want to let through TEMPLATE_TYPE_PARMs 

>>>>>>>> representing 'auto'

>>>>>>>> - otherwise Dodji's check a few lines below which fixed c++/51473

>>>>>>>> doesn't work anymore - and also BOUND_TEMPLATE_TEMPLATE_PARM, 

>>>>>>>> otherwise

>>>>>>>> we regress on template/spec32.C and template/ttp22.C because we 

>>>>>>>> don't

>>>>>>>> diagnose the shadowing anymore. Thus, I would say either we 

>>>>>>>> keep on

>>>>>>>> using MAYBE_CLASS_TYPE_P or we pick what we need, possibly we 

>>>>>>>> add a comment?

>>>>>>> Aha.  I guess the answer is not to restrict that test any more, but

>>>>>>> instead to fix the code further down so it gives a proper 

>>>>>>> diagnostic

>>>>>>> rather than call warn_misplaced_attr_for_class_type.

>>>>>>

>>>>>> I see. Thus something like the below? It passes testing on 

>>>>>> x86_64-linux.

>>>>>

>>>>>> +  if ((!declared_type || TREE_CODE (declared_type) == 

>>>>>> DECLTYPE_TYPE)

>>>>>> +      && ! saw_friend && !error_p)

>>>>>>      permerror (input_location, "declaration does not declare 

>>>>>> anything");

>>>>>

>>>>> I see no reason to make this specific to decltype.  Maybe move 

>>>>> this diagnostic into the final 'else' block with the other 

>>>>> declspec diagnostics and not look at declared_type at all?

>>>>

>>>> I'm not sure to fully understand: if we do that we still want to at 

>>>> least minimally check that declared_type is null, like we already 

>>>> do, and then we simply accept the new testcase. Is that Ok? 

>>>> Because, as I probably mentioned at some point, all the other 

>>>> compilers I have at hand issue a "does not declare anything" 

>>>> diagnostic, and we likewise do that for the legacy __typeof. Not 

>>>> looking into declared_type *at all* doesn't work with plain class 

>>>> types and enums, of course. Or you meant something entirely 

>>>> different??

>>>>

>>>>>> +  if (declspecs->attributes && warn_attributes && declared_type

>>>>>> +      && TREE_CODE (declared_type) != DECLTYPE_TYPE)

>>>>>

>>>>> I think we do want to give a diagnostic about useless attributes, 

>>>>> not skip it.

>>>>

>>>> Agreed. FWIW the attached tests fine.

>>>

>>> The problem here is that the code toward the bottom expects 

>>> "declared_type" to be the tagged type declared by a declaration with 

>>> no declarator, and in this testcase it's ending up as a DECLTYPE_TYPE.

>>>

>>> I think once we've checked for 'auto' we don't want declared_type to 

>>> be anything that isn't OVERLOAD_TYPE_P.  We can arrange that either 

>>> by checking for 'auto' first and then changing the code that sets 

>>> declared_type to use OVERLOAD_TYPE_P, or by clearing declared_type 

>>> after checking for 'auto' if it isn't OVERLOAD_TYPE_P.

>>

>> Thanks. I'm slowly catching up on this issue... Any suggestion about 

>> BOUND_TEMPLATE_TEMPLATE_PARM? If we don't let through such tree nodes 

>> - which are MAYBE_CLASS_TYPE_P and aren't OVERLOAD_TYPE_P - we 

>> regress on template/spec32.C, we don't reject it anymore.

> If we clear declared_type for a BOUND_TEMPLATE_TEMPLATE_PARM, we 

> should get the "does not declare anything" error.


Ah, now I see, I didn't realize that we would also change the errors we 
issue for those testcases. Thus the below is finishing testing, appears 
to work fine.

Thanks, Paolo.

///////////////////
Index: cp/decl.c
===================================================================
--- cp/decl.c	(revision 267131)
+++ cp/decl.c	(working copy)
@@ -4803,9 +4803,8 @@ check_tag_decl (cp_decl_specifier_seq *declspecs,
     declared_type = declspecs->type;
   else if (declspecs->type == error_mark_node)
     error_p = true;
-  if (declared_type == NULL_TREE && ! saw_friend && !error_p)
-    permerror (input_location, "declaration does not declare anything");
-  else if (declared_type != NULL_TREE && type_uses_auto (declared_type))
+
+  if (type_uses_auto (declared_type))
     {
       error_at (declspecs->locations[ds_type_spec],
 		"%<auto%> can only be specified for variables "
@@ -4812,6 +4811,12 @@ check_tag_decl (cp_decl_specifier_seq *declspecs,
 		"or function declarations");
       return error_mark_node;
     }
+
+  if (declared_type && !OVERLOAD_TYPE_P (declared_type))
+    declared_type = NULL_TREE;
+
+  if (!declared_type && !saw_friend && !error_p)
+    permerror (input_location, "declaration does not declare anything");
   /* Check for an anonymous union.  */
   else if (declared_type && RECORD_OR_UNION_CODE_P (TREE_CODE (declared_type))
 	   && TYPE_UNNAMED_P (declared_type))
Index: testsuite/g++.dg/cpp0x/decltype-33838.C
===================================================================
--- testsuite/g++.dg/cpp0x/decltype-33838.C	(revision 267127)
+++ testsuite/g++.dg/cpp0x/decltype-33838.C	(working copy)
@@ -2,5 +2,5 @@
 // PR c++/33838
 template<typename T> struct A
 {
-  __decltype (T* foo()); // { dg-error "expected|no arguments|accept" }
+  __decltype (T* foo()); // { dg-error "expected|no arguments|declaration" }
 };
Index: testsuite/g++.dg/cpp0x/decltype68.C
===================================================================
--- testsuite/g++.dg/cpp0x/decltype68.C	(nonexistent)
+++ testsuite/g++.dg/cpp0x/decltype68.C	(working copy)
@@ -0,0 +1,7 @@
+// PR c++/84644
+// { dg-do compile { target c++11 } }
+
+template<int a>
+struct b {
+  decltype(a) __attribute__((break));  // { dg-error "declaration does not declare anything" }
+};
Index: testsuite/g++.dg/template/spec32.C
===================================================================
--- testsuite/g++.dg/template/spec32.C	(revision 267127)
+++ testsuite/g++.dg/template/spec32.C	(working copy)
@@ -2,5 +2,5 @@
 
 struct A
 {
-  template<template<int> class B> struct B<0>;  // { dg-error "name of class shadows" }
+  template<template<int> class B> struct B<0>;  // { dg-error "declaration does not declare anything" }
 };
Index: testsuite/g++.dg/template/ttp22.C
===================================================================
--- testsuite/g++.dg/template/ttp22.C	(revision 267127)
+++ testsuite/g++.dg/template/ttp22.C	(working copy)
@@ -2,7 +2,7 @@
 // { dg-do compile }
 
 template<template<int> class A>
-class A<0>;  // { dg-error "shadows template template parameter" }
+class A<0>;  // { dg-error "declaration does not declare anything" }
 
 template<template<int> class B>
 class B<0> {};  // { dg-error "shadows template template parameter" }
Jason Merrill Dec. 14, 2018, 9:43 p.m. | #11
On 12/14/18 4:33 PM, Paolo Carlini wrote:
> Hi,

> 

> On 14/12/18 21:19, Jason Merrill wrote:

>> On 12/14/18 1:44 PM, Paolo Carlini wrote:

>>> Hi,

>>>

>>> On 13/12/18 22:03, Jason Merrill wrote:

>>>> On 10/30/18 9:22 PM, Paolo Carlini wrote:

>>>>> Hi,

>>>>>

>>>>> On 30/10/18 21:37, Jason Merrill wrote:

>>>>>> On 10/26/18 2:02 PM, Paolo Carlini wrote:

>>>>>>> On 26/10/18 17:18, Jason Merrill wrote:

>>>>>>>> On Fri, Oct 26, 2018 at 4:52 AM Paolo Carlini 

>>>>>>>> <paolo.carlini@oracle.com> wrote:

>>>>>>>>> On 24/10/18 22:41, Jason Merrill wrote:

>>>>>>>>>> On 10/15/18 12:45 PM, Paolo Carlini wrote:

>>>>>>>>>>>          && ((TREE_CODE (declspecs->type) != TYPENAME_TYPE

>>>>>>>>>>> +       && TREE_CODE (declspecs->type) != DECLTYPE_TYPE

>>>>>>>>>>>           && MAYBE_CLASS_TYPE_P (declspecs->type))

>>>>>>>>>> I would think that the MAYBE_CLASS_TYPE_P here should be 

>>>>>>>>>> CLASS_TYPE_P,

>>>>>>>>>> and then we can remove the TYPENAME_TYPE check. Or do we want to

>>>>>>>>>> allow template type parameters for some reason?

>>>>>>>>> Indeed, it would be nice to just use OVERLOAD_TYPE_P. However 

>>>>>>>>> it seems

>>>>>>>>> we at least want to let through TEMPLATE_TYPE_PARMs 

>>>>>>>>> representing 'auto'

>>>>>>>>> - otherwise Dodji's check a few lines below which fixed c++/51473

>>>>>>>>> doesn't work anymore - and also BOUND_TEMPLATE_TEMPLATE_PARM, 

>>>>>>>>> otherwise

>>>>>>>>> we regress on template/spec32.C and template/ttp22.C because we 

>>>>>>>>> don't

>>>>>>>>> diagnose the shadowing anymore. Thus, I would say either we 

>>>>>>>>> keep on

>>>>>>>>> using MAYBE_CLASS_TYPE_P or we pick what we need, possibly we 

>>>>>>>>> add a comment?

>>>>>>>> Aha.  I guess the answer is not to restrict that test any more, but

>>>>>>>> instead to fix the code further down so it gives a proper 

>>>>>>>> diagnostic

>>>>>>>> rather than call warn_misplaced_attr_for_class_type.

>>>>>>>

>>>>>>> I see. Thus something like the below? It passes testing on 

>>>>>>> x86_64-linux.

>>>>>>

>>>>>>> +  if ((!declared_type || TREE_CODE (declared_type) == 

>>>>>>> DECLTYPE_TYPE)

>>>>>>> +      && ! saw_friend && !error_p)

>>>>>>>      permerror (input_location, "declaration does not declare 

>>>>>>> anything");

>>>>>>

>>>>>> I see no reason to make this specific to decltype.  Maybe move 

>>>>>> this diagnostic into the final 'else' block with the other 

>>>>>> declspec diagnostics and not look at declared_type at all?

>>>>>

>>>>> I'm not sure to fully understand: if we do that we still want to at 

>>>>> least minimally check that declared_type is null, like we already 

>>>>> do, and then we simply accept the new testcase. Is that Ok? 

>>>>> Because, as I probably mentioned at some point, all the other 

>>>>> compilers I have at hand issue a "does not declare anything" 

>>>>> diagnostic, and we likewise do that for the legacy __typeof. Not 

>>>>> looking into declared_type *at all* doesn't work with plain class 

>>>>> types and enums, of course. Or you meant something entirely 

>>>>> different??

>>>>>

>>>>>>> +  if (declspecs->attributes && warn_attributes && declared_type

>>>>>>> +      && TREE_CODE (declared_type) != DECLTYPE_TYPE)

>>>>>>

>>>>>> I think we do want to give a diagnostic about useless attributes, 

>>>>>> not skip it.

>>>>>

>>>>> Agreed. FWIW the attached tests fine.

>>>>

>>>> The problem here is that the code toward the bottom expects 

>>>> "declared_type" to be the tagged type declared by a declaration with 

>>>> no declarator, and in this testcase it's ending up as a DECLTYPE_TYPE.

>>>>

>>>> I think once we've checked for 'auto' we don't want declared_type to 

>>>> be anything that isn't OVERLOAD_TYPE_P.  We can arrange that either 

>>>> by checking for 'auto' first and then changing the code that sets 

>>>> declared_type to use OVERLOAD_TYPE_P, or by clearing declared_type 

>>>> after checking for 'auto' if it isn't OVERLOAD_TYPE_P.

>>>

>>> Thanks. I'm slowly catching up on this issue... Any suggestion about 

>>> BOUND_TEMPLATE_TEMPLATE_PARM? If we don't let through such tree nodes 

>>> - which are MAYBE_CLASS_TYPE_P and aren't OVERLOAD_TYPE_P - we 

>>> regress on template/spec32.C, we don't reject it anymore.

>> If we clear declared_type for a BOUND_TEMPLATE_TEMPLATE_PARM, we 

>> should get the "does not declare anything" error.

> 

> Ah, now I see, I didn't realize that we would also change the errors we 

> issue for those testcases. Thus the below is finishing testing, appears 

> to work fine.


OK.

Jason

Patch

Index: cp/decl.c
===================================================================
--- cp/decl.c	(revision 265158)
+++ cp/decl.c	(working copy)
@@ -4793,6 +4793,7 @@  check_tag_decl (cp_decl_specifier_seq *declspecs,
   if (declspecs->type
       && TYPE_P (declspecs->type)
       && ((TREE_CODE (declspecs->type) != TYPENAME_TYPE
+	   && TREE_CODE (declspecs->type) != DECLTYPE_TYPE
 	   && MAYBE_CLASS_TYPE_P (declspecs->type))
 	  || TREE_CODE (declspecs->type) == ENUMERAL_TYPE))
     declared_type = declspecs->type;
Index: testsuite/g++.dg/cpp0x/decltype-33838.C
===================================================================
--- testsuite/g++.dg/cpp0x/decltype-33838.C	(revision 265158)
+++ testsuite/g++.dg/cpp0x/decltype-33838.C	(working copy)
@@ -2,5 +2,5 @@ 
 // PR c++/33838
 template<typename T> struct A
 {
-  __decltype (T* foo()); // { dg-error "expected|no arguments|accept" }
+  __decltype (T* foo()); // { dg-error "expected|no arguments|declaration" }
 };
Index: testsuite/g++.dg/cpp0x/decltype68.C
===================================================================
--- testsuite/g++.dg/cpp0x/decltype68.C	(nonexistent)
+++ testsuite/g++.dg/cpp0x/decltype68.C	(working copy)
@@ -0,0 +1,7 @@ 
+// PR c++/84644
+// { dg-do compile { target c++11 } }
+
+template<int a>
+struct b {
+  decltype(a) __attribute__((break));  // { dg-error "declaration does not declare anything" }
+};