| 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") | expand |
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
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.
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
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" } +};
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
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); }
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
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.
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
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" }
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
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" } +};