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

[Paolo Carlini <paolo dot carlini at oracle dot com>]

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.

///////////////////

Attachment: patch_84644_3
Description: Text document