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

[Jason Merrill <jason@redhat.com>]

On 3/26/20 2:58 PM, Martin Sebor wrote:
> On 3/25/20 11:36 PM, Jason Merrill wrote:
>> 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.
> 
> I would prefer to get this patch committed as is now.  I appreciate
> there are improvements that can be made to the code (there always
> are) but, unlike the bugs it fixes, they are invisible to users and
> so don't seem essential at this point.
> 
>>> +  /* 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.
> 
> Okay.
> 
>>
>> 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.
> 
> Looks like the test is an unnecessary remnant and can be removed.
> In fact, both is_decl() and decl_p member don't appear necessary
> anymore so I've removed them too.
> 
>>> +     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;
> 
> Obviously, just like a class.  Is there something you expect me to
> change in response to this point?

You're hardcoding index zero for the template case, which seems to 
assume that the definition of a template is always at index zero.

>> 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.
> 
> I'm not quite sure what you have in mind here.  I agree the cdlcode
> code looks a little cumbersome and perhaps could be restructured but
> it's not obvious to me how.  Nothing I tried looked like a clear win
> so unless you consider changing how this is done a prerequisite for
> accepting the whole patch I'd rather not spend any more time at this
> stage iterating over refinements of it.  Please let me know soon.

I mean that

> +  const unsigned ndecls = locvec.length (); > +  const bool def_p = idxdef < ndecls;
> +  const unsigned idxguide = def_p ? idxdef : 0;

are based on whether the instantiation, rather than the template, is 
defined.

I's probably enough to update ndecls to cdlguide->locvec.length() and 
change the uses of idxdef to cdlguide->idxdef.  And then idxguide will 
be set properly for cdlguide, so the code farther above can become

> +  const tag_types xpect_key
> +    = cdlguide->class_key (idxguide);

If you'd prefer, I can make these changes and commit the patch myself.

Jason