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

[Martin Sebor <msebor@gmail.com>]

On 3/26/20 4:16 PM, Jason Merrill wrote:
> 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.

Sounds like you're concerned about something like:

   template <class> class A;
   template <class T> class A<T*>;        // expect -Wmismatched-tags
   template <class T> struct A<T*> { };   // ...because of this
   class A<int*> a;                       // expect -Wmismatched-tags

The definition should be at index zero because once it's seen, entries
for prior declarations are purged (after diagnosing mismatches).  But
I'm sure the tests for these things could stand to beefed up so there
could easily be cases that aren't handled correctly.

> 
>>> 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.

Please go ahead.  That will make it a lot quicker.

Martin