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

[Martin Sebor]

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

On 3/12/20 4:38 PM, Martin Sebor wrote:
> On 3/12/20 11:03 AM, Martin Sebor wrote:
>> On 3/11/20 3:30 PM, Martin Sebor wrote:
>>> On 3/11/20 2:10 PM, Jason Merrill wrote:
>>>> On 3/11/20 12:57 PM, Martin Sebor wrote:
>>>>> On 3/9/20 6:08 PM, Jason Merrill wrote:
>>>>>> On 3/9/20 5:39 PM, Martin Sebor wrote:
>>>>>>> On 3/9/20 1:40 PM, Jason Merrill wrote:
>>>>>>>> On 3/9/20 12:31 PM, Martin Sebor wrote:
>>>>>>>>> On 2/28/20 1:24 PM, Jason Merrill wrote:
>>>>>>>>>> On 2/28/20 12:45 PM, Martin Sebor wrote:
>>>>>>>>>>> On 2/28/20 9:58 AM, Jason Merrill wrote:
>>>>>>>>>>>> On 2/24/20 6:58 PM, Martin Sebor wrote:
>>>>>>>>>>>>> -Wredundant-tags doesn't consider type declarations that 
>>>>>>>>>>>>> are also
>>>>>>>>>>>>> the first uses of the type, such as in 'void f (struct S);' 
>>>>>>>>>>>>> and
>>>>>>>>>>>>> issues false positives for those.  According to the 
>>>>>>>>>>>>> reported that's
>>>>>>>>>>>>> making it harder to use the warning to clean up LibreOffice.
>>>>>>>>>>>>>
>>>>>>>>>>>>> The attached patch extends -Wredundant-tags to avoid these 
>>>>>>>>>>>>> false
>>>>>>>>>>>>> positives by relying on the same 
>>>>>>>>>>>>> class_decl_loc_t::class2loc mapping
>>>>>>>>>>>>> as -Wmismatched-tags.  The patch also somewhat improves the 
>>>>>>>>>>>>> detection
>>>>>>>>>>>>> of both issues in template declarations (though more work 
>>>>>>>>>>>>> is still
>>>>>>>>>>>>> needed there).
>>>>>>>>>>>>
>>>>>>>>>>>>> +         a new entry for it and return unless it's a 
>>>>>>>>>>>>> declaration
>>>>>>>>>>>>> +         involving a template that may need to be 
>>>>>>>>>>>>> diagnosed by
>>>>>>>>>>>>> +         -Wredundant-tags.  */
>>>>>>>>>>>>>        *rdl = class_decl_loc_t (class_key, false, def_p);
>>>>>>>>>>>>> -      return;
>>>>>>>>>>>>> +      if (TREE_CODE (decl) != TEMPLATE_DECL)
>>>>>>>>>>>>> +        return;
>>>>>>>>>>>>
>>>>>>>>>>>> How can the first appearance of a class template be redundant?
>>>>>>>>>>>
>>>>>>>>>>> I'm not sure I correctly understand the question.  The 
>>>>>>>>>>> comment says
>>>>>>>>>>> "involving a template" (i.e., not one of the first 
>>>>>>>>>>> declaration of
>>>>>>>>>>> a template).  The test case that corresponds to this test is:
>>>>>>>>>>>
>>>>>>>>>>>    template <class> struct S7 { };
>>>>>>>>>>>    struct S7<void> s7v;  // { dg-warning 
>>>>>>>>>>> "\\\[-Wredundant-tags" }
>>>>>>>>>>>
>>>>>>>>>>> where DECL is the TEPLATE_DECL of S7<void>.
>>>>>>>>>>>
>>>>>>>>>>> As I mentioned, more work is still needed to handle templates 
>>>>>>>>>>> right
>>>>>>>>>>> because some redundant tags are still not diagnosed.  For 
>>>>>>>>>>> example:
>>>>>>>>>>>
>>>>>>>>>>>    template <class> struct S7 { };
>>>>>>>>>>>    template <class T>
>>>>>>>>>>>    using U = struct S7<T>;   // missing warning
>>>>>>>>>>
>>>>>>>>>> When we get here for an instance of a template, it doesn't 
>>>>>>>>>> make sense to treat it as a new type.
>>>>>>>>>>
>>>>>>>>>> If decl is a template and type_decl is an instance of that 
>>>>>>>>>> template, do we want to (before the lookup) change type_decl 
>>>>>>>>>> to the template or the corresponding generic TYPE_DECL, which 
>>>>>>>>>> should already be in the table?
>>>>>>>>>
>>>>>>>>> I'm struggling with how to do this.  Given type (a RECORD_TYPE) 
>>>>>>>>> and
>>>>>>>>> type_decl (a TEMPLATE_DECL) representing the use of a template, 
>>>>>>>>> how
>>>>>>>>> do I get the corresponding template (or its explicit or partial
>>>>>>>>> specialization) in the three cases below?
>>>>>>>>>
>>>>>>>>>    1) Instance of the primary:
>>>>>>>>>       template <class> class A;
>>>>>>>>>       struct A<int> a;
>>>>>>>>>
>>>>>>>>>    2) Instance of an explicit specialization:
>>>>>>>>>       template <class> class B;
>>>>>>>>>       template <> struct B<int>;
>>>>>>>>>       class B<int> b;
>>>>>>>>>
>>>>>>>>>    3) Instance of a partial specialization:
>>>>>>>>>       template <class> class C;
>>>>>>>>>       template <class T> struct C<T*>;
>>>>>>>>>       class C<int*> c;
>>>>>>>>>
>>>>>>>>> By trial and (lots of) error I figured out that in both (1) and 
>>>>>>>>> (2),
>>>>>>>>> but not in (3), TYPE_MAIN_DECL (TYPE_TI_TEMPLATE (type)) returns
>>>>>>>>> the template's type_decl.
>>>>>>>>>
>>>>>>>>> Is there some function to call to get it in (3), or even better,
>>>>>>>>> in all three cases?
>>>>>>>>
>>>>>>>> I think you're looking for most_general_template.
>>>>>>>
>>>>>>> I don't think that's quite what I'm looking for.  At least it 
>>>>>>> doesn't
>>>>>>> return the template or its specialization in all three cases above.
>>>>>>
>>>>>> Ah, true, that function stops at specializations.  Oddly, I don't 
>>>>>> think there's currently a similar function that looks through 
>>>>>> them. You could create one that does a simple loop through 
>>>>>> DECL_TI_TEMPLATE like is_specialization_of.
>>>>>
>>>>> Thanks for the tip.  Even with that I'm having trouble with partial
>>>>> specializations.  For example in:
>>>>>
>>>>>    template <class>   struct S;
>>>>>    template <class T> class S<const T>;
>>>>>    extern class  S<const int> s1;
>>>>>    extern struct S<const int> s2;  // expect -Wmismatched-tags
>>>>>
>>>>> how do I find the declaration of the partial specialization when given
>>>>> the type in the extern declaration?  A loop in my find_template_for()
>>>>> function (similar to is_specialization_of) only visits the implicit
>>>>> specialization S<const int> (i.e., its own type) and the primary.
>>>>
>>>> Is that a problem?  The name is from the primary template, so does 
>>>> it matter for this warning whether there's an explicit 
>>>> specialization involved?
>>>
>>> I don't understand the question.  S<const int> is an instance of
>>> the partial specialization.  To diagnose the right mismatch the warning
>>> needs to know how to find the template (i.e., either the primary, or
>>> the explicit or partial specialization) the instance corresponds to and
>>> the class-key it was declared with.  As it is, while GCC does diagnose
>>> the right declaration (that of s2), it does that thanks to a bug:
>>> because it finds and uses the type and class-key used to declare s1.
>>> If we get rid of s1 it doesn't diagnose anything.
>>>
>>> I tried using DECL_TEMPLATE_SPECIALIZATIONS() to get the list of
>>> the partial specializations but it doesn't like any of the arguments
>>> I've given it (it ICEs).
>>
>> With this fixed, here's the algorithm I tried:
>>
>> 1) for a type T of a template instantiation (s1 above), get the primary
>>     P that T was instantiated from using
>>     P = TYPE_MAIN_DECL (CLASSTYPE_PRIMARY_TEMPLATE_TYPE (T)),
>> 2) from P, get the chain of its specializations using
>>     SC = DECL_TEMPLATE_SPECIALIZATIONS (P)
>> 3) for each (partial) specialization S on the SC chain get the chain
>>     of its instantiations IC using DECL_TEMPLATE_INSTANTIATIONS, if
>>     is_specialization_of (T, TREE_VALUE (IC)) is non-zero take
>>     TREE_VALUE (SC) as the declaration of the partial specialization
>>     that the template instanstantiaton T was generated from.
>>
>> Unfortunately, in the example above, DECL_TEMPLATE_INSTANTIATIONS for
>> the partial specialization 'class S<const T>' is null (even after all
>> the declarations have been parsed) so I'm at a dead end.
> 
> After a lot more trial and error I discovered
> most_specialized_partial_spec in pt.c with whose help I have been able
> to get templates to work the way I think they should (at least the cases
> I've tested do).
> 
> Besides fixing the original problem that motivated it, the attached
> patch also corrects how template specializations are handled: the first
> declaration of either a primary template or its specialization (either
> explicit or partial) determines the class-key in subsequent uses of
> the type or its instantiations.  To do this for uses of first-time
> template instantiations such as in the declaration of s1 in the test
> case above, class_decl_loc_t::diag_mismatched_tags looks up the template
> (either the primary or the partial specialization) in the CLASS2LOC map
> and uses it and its class-key as the guide when issuing diagnostics.
> This also means that the first instance of every template needs to
> have a record in the CLASS2LOC map (it also needs it to compare its
> class-key to subsequent redeclarations of the template).
> 
> This has been unexpectedly difficult.  A big part of it is that I've
> never before worked with templates in the front-end so I had to learn
> how they're organized (I'm far from having mastered it).  What's made
> the learning curve especially steep, besides the sparse documentation,
> is the problems hinted at in the paragraph below below.  This whole
> area could really stand to be documented in some sort of a writeup:
> a high-level overview of how templates are put together (i.e., what
> hangs off what in the tree representation) and what APIs to use to
> work with them.
> 
> The revised patch has been tested on x86_64-linux.
> 
> Martin
> 
> 
>> The other recurring problem I'm running into is that many of the C++
>> FE macros (and APIs) don't always return the expected/documented result.
>> I think this is at least in part because until a declaration has been
>> fully parsed not all the bits are in place to make it possible to tell
>> if it's an implicit or explicit specialization, for example (so
>> CLASSTYPE_USE_TEMPLATE (T) is 1 for the first-time declaration of
>> an explicit specialization, for example).  But in view of the problem
>> above I'm not sure that's the only reason.
>>
>>>
>>> Martin
>>>
>>> PS As an aside, both Clang and VC++ have trouble with templates as
>>> well, just slightly different kinds of trouble.   Clang diagnoses
>>> the declaration of the partial specialization while VC++ diagnoses
>>> s1.  In other similar cases like the one below VC++ does the right
>>> thing and Clang is silent.
>>>
>>>    template <class>   struct S { };
>>>    template <class T> class S<const T> { };
>>>
>>>    template <class T> void f (class S<T>);          // VC++ warns
>>>    template <class T> void g (struct S<const T>);   // GCC & VC++ warn
>>>
>>>>
>>>>> Martin
>>>>>
>>>>>>
>>>>>>> In (2) and (3) it won't distinguish between specializations of B or
>>>>>>> C on different types.  In (2), the function returns the same result
>>>>>>> for both:
>>>>>>>
>>>>>>>    template <> struct B<int>;
>>>>>>>    template <> struct B<char>;
>>>>>>>
>>>>>>> In (3), it similarly returns the same result for both of
>>>>>>>
>>>>>>>    template <class T> struct C<T*>;
>>>>>>>    template <class T> struct C<const T>;
>>>>>>>
>>>>>>> even though they are declarations of distinct types.
>>>>>>
>>>>>>
>>>>>> Jason
>>>>>>
>>>>>
>>>>
>>>
>>
>