Re: [PATCH] avoid -Wredundant-tags on a first declaration in use (PR 93824)

[Jason Merrill <jason@redhat.com>]

On 3/23/20 12:50 PM, Martin Sebor wrote:
> On 3/23/20 8:49 AM, Jason Merrill wrote:
>> On 3/21/20 5:59 PM, Martin Sebor wrote:
>>> +      /* Diagnose class/struct/union mismatches.  IS_DECLARATION is 
>>> false
>>> +     for alias definition.  */
>>> +      bool decl_class = (is_declaration
>>> +             && cp_parser_declares_only_class_p (parser));
>>>         cp_parser_check_class_key (parser, key_loc, tag_type, type, 
>>> false,
>>>                    cp_parser_declares_only_class_p (parser));
>>
>> Don't you need to use the new variable?
>>
>> Don't your testcases exercise this?
> 
> Of course they do.  This was a leftover from an experiment after I put
> the initial updated patch together.  On final review I decided to adjust
> some comments and forgot to restore the original use of the variable.
> 
>>> +      /* When TYPE is the use of an implicit specialization of a 
>>> previously
>>> +     declared template set TYPE_DECL to the type of the primary 
>>> template
>>> +     for the specialization and look it up in CLASS2LOC below.  For 
>>> uses
>>> +     of explicit or partial specializations TYPE_DECL already points to
>>> +     the declaration of the specialization.
>>> +     IS_USE is clear so that the type of an implicit instantiation 
>>> rather
>>> +     than that of a partial specialization is determined.  */
>>> +      type_decl = TREE_TYPE (type_decl);
>>> +      if (TREE_CODE (type_decl) != TEMPLATE_DECL)
>>> +    type_decl = TYPE_MAIN_DECL (type_decl);
>>
>> The comment is no longer relevant to the code.  The remaining code 
>> also seems like it would have no effect; we already know type_decl is 
>> TYPE_MAIN_DECL (type).
> 
> I removed the block of code.
> 
> Martin
> 
> PS I would have preferred to resolve just the reported problem in this
> patch and deal with the template specializations more fully (and with
> aliases) in a followup.  As it is, it has grown bigger and more complex
> than I'm comfortable with, especially with the template specializations,
> harder for me to follow, and obviously a lot more time-consuming not
> just to put together but also to review.  Although this revision handles
> many more template specialization cases correctly, there still are other
> (arguably corner) cases that it doesn't.  I suspect getting those right
> might even require a design change, which I see as out of scope at this
> time (not to mention my ability).

Sure, at this point in the cycle there's always a tradeoff between 
better functionality and risk from ballooning changes.  It looks like 
the improved template handling could still be split out into a separate 
patch, if you'd prefer.

> +  /* Number of usesn of the class.  */
Typo.

> +     definintion if one exists or the first declaration otherwise.  */
typo.

> +  if (CLASSTYPE_USE_TEMPLATE (type) == 1 && !is_decl (0))
...
> +	 the first reference to the instantiation.  The primary must
> +	 be (and inevitably is) at index zero.  */

I think CLASSTYPE_IMPLICIT_INSTANTIATION is more readable than testing 
the value 1.

What is the !is_decl (0) intended to do?  Changing it to an assert 
inside the 'if' doesn't seem to affect any of the testcases.

> +     For implicit instantiations of a primary template it's
> +     the class-key used to declare the primary with.  The primary
> +     must be at index zero.  */
> +  const tag_types xpect_key
> +    = cdlguide->class_key (cdlguide == this ? idxguide : 0);

A template can also be declared before it's defined; I think you want to 
move the def_p/idxdef/idxguide logic into another member function that 
you invoke on cdlguide to perhaps get the class_key_loc_t that you want 
to use as the pattern.

Jason