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

[Martin Sebor <msebor@gmail.com>]

Ping: Jason, is the latest patch acceptable or are there any other
changes you want me to make?

   https://gcc.gnu.org/pipermail/gcc-patches/2020-March/542538.html

On 3/21/20 3:59 PM, Martin Sebor wrote:
> On 3/20/20 3:53 PM, Jason Merrill wrote:
>> On 3/19/20 7:55 PM, Martin Sebor wrote:
>>> On 3/18/20 9:07 PM, Jason Merrill wrote:
>>>> On 3/12/20 6:38 PM, Martin Sebor wrote:
>>> ...
>>>>> +     declarations of a class from its uses doesn't work for type 
>>>>> aliases
>>>>> +     (as in using T = class C;).  */
>>>>
>>>> Good point.  Perhaps we could pass flags to 
>>>> cp_parser_declares_only_class_p and have it return false if 
>>>> CP_PARSER_FLAGS_TYPENAME_OPTIONAL, since that is set for an alias 
>>>> but not for a normal type-specifier.
>>>
>>> I wondered if there was a way to identify that we're dealing with
>>> an alias.  CP_PARSER_FLAGS_TYPENAME_OPTIONAL is set not just for
>>> those but also for template declarations (in
>>> cp_parser_single_declaration) but I was able to make it work by
>>> tweaking cp_parser_type_specifier.  It doesn't feel very clean
>>> (it seems like either the bit or all of cp_parser_flags could be
>>> a member of the parser class or some subobject of it) but it does
>>> the job.  Thanks for pointing me in the right direction!
>>
>> Hmm, true, relying on that flag is probably too fragile.  And now that 
>> I look closer, I see that we already have is_declaration in 
>> cp_parser_elaborated_type_specifier, we just need to check that before 
>> cp_parser_declares_only_class_p like we do earlier in the function.
> 
> I changed it to use is_declaration instead.
> 
>>>>> +  if (!decl_p && !def_p && TREE_CODE (decl) == TEMPLATE_DECL)
>>>>>      {
>>>>> +      /* 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.  */
>>>>> +      type_decl = specialization_of (type_decl);
>>>>
>>>> Here shouldn't is_use be true?
>>>
>>> If it were set to true here we would find the partial specialization
>>> corresponding to its specialization in the use when what we want is
>>> the latter.  As a result, for the following:
>>>
>>>    template <class>   struct S;
>>>    template <class T> struct S<T*>;
>>>
>>>    extern class  S<int*> s1;   // expect -Wmismatched-tags
>>>    extern struct S<int*> s2;
>>>
>>> we'd end up with a warning for s2 pointing to the instantiation of
>>> s1 as the "guiding declaration:"
>>>
>>> z.C:5:15: warning: ‘template<class T> struct S<T*>’ declared with a 
>>> mismatched class-key ‘struct’ [-Wmismatched-tags]
>>>      5 | extern struct S<int*> s2;
>>>        |               ^~~~~~~
>>> z.C:5:15: note: remove the class-key or replace it with ‘class’
>>> z.C:4:15: note: ‘template<class T> struct S<T*>’ first declared as 
>>> ‘class’ here
>>>      4 | extern class  S<int*> s1;   // expect -Wmismatched-tags
>>>        |               ^~~~~~~
>>
>> I found this puzzling and wanted to see why that would be, but I can't 
>> reproduce it; compiling with -Wmismatched-tags produces only
> 
> I'm not sure what you did differently.  With the patch (the last one
> or the one in the attachment) we get the expected warning below.
> 
>>
>> wa2.C:4:17: warning: ‘S<T*>’ declared with a mismatched class-key 
>> ‘class’ [-Wmismatched-tags]
>>      4 |   extern class  S<int*> s1;   // expect -Wmismatched-tags
>>        |                 ^~~~~~~
>> wa2.C:4:17: note: remove the class-key or replace it with ‘struct’
>> wa2.C:2:29: note: ‘S<T*>’ first declared as ‘struct’ here
>>      2 |   template <class T> struct S<T*>;
>>
>> So the only difference is whether we talk about S<T*> or S<int*>.  I 
>> agree that the latter is probably better, which is what you get 
>> without my suggested change.  But since specialization_of does nothing 
>> if is_use is false, how about removing the call here and removing the 
>> is_use parameter?
> 
> Sure.
> 
>>
>>> +  if (tree spec = most_specialized_partial_spec (ret, tf_none))
>>> +    if (spec != error_mark_node)
>>> +      ret = TREE_VALUE (spec);
>>
>> I think you want to take the TREE_TYPE of the template here, so you 
>> don't need to do it here:
>>
>>> +      tree pt = specialization_of (TYPE_MAIN_DECL (type), true);
>>> +      if (TREE_CODE (pt) == TEMPLATE_DECL)
>>> +       pt = TREE_TYPE (pt);
>>> +      pt = TYPE_MAIN_DECL (pt);
>>
>> And also, since it takes a TYPE_DECL, it would be better to return 
>> another TYPE_DECL rather than a _TYPE, especially since the only 
>> caller immediately extracts a TYPE_DECL.
> 
> Okay.  Attached is an revised patch with these changes.
> 
> Martin