[PATCH] Restrict -Winconsistent-missing-override warnings on class templates with dependent bases

[David Blaikie]

On Fri, Mar 6, 2015 at 3:31 PM, jahanian <fjahanian at apple.com> wrote:

>
> On Mar 6, 2015, at 3:15 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> On Fri, Mar 6, 2015 at 2:57 PM, jahanian <fjahanian at apple.com> wrote:
>
>>
>> On Mar 6, 2015, at 2:47 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>
>>
>>
>> On Fri, Mar 6, 2015 at 2:45 PM, jahanian <fjahanian at apple.com> wrote:
>>
>>>
>>> On Mar 6, 2015, at 9:36 AM, David Blaikie <dblaikie at gmail.com> wrote:
>>>
>>> Would it be plausible to check this on templates directly, rather than
>>> on their instantiations? This would be less work in the case of multiple
>>> instantiations, avoid redundant diagnostics, fail on templates without
>>> instantiations rather than creating a lurking failure, and we might even
>>> get all the "dependent" tests for free - because we wouldn't be able to
>>> look through the dependent types at all.
>>>
>>>
>>> It could be plausible. But, in similar cases, checking is done on the
>>> instantiated templates and not on the templates directly. This adds another
>>> check in the
>>> same code block. Providing a new iteration on templates for this one
>>> check is prohibitively expensive (and we normally don’t do much checking on
>>> templates).
>>>
>>
>> It is? I'd be curious to see the numbers, as it sounds like you have some.
>>
>>
>> No I don’t have any. But iterating over templates looking for methods
>> would add to cost.
>>
>
> Well, if we skipped template instantiations it wouldn't necessarily be a
> strict increase - we'd only visit the template pattern, and none of the
> instantiations.
>
>
> We already visit template instantiations for issuing other diagnostics.
> New overhead is minimal comparing to iterating over template declarations
> looking for methods.
>

I don't quite follow - in this instance, given one instantiation of a
template, we'd trade one visitation for another - the entirety
of DiagnoseAbsenceOfOverrideControl would skip template instantiations and
then be called for each template pattern.


>
>
>
>> Are we iterating over templates for other diagnostics?
>>
> If so, I can add this there.
>>
>
> I don't recall any particular cases for/against.
>
>
>>
>>
>>
>>> Do you see anything inherently wrong to adding this check where it is?
>>>
>>
>> Just the issues I mentioned - duplicate diagnostics in the case of
>> multiple instantiations (& no diagnostics in the case of no instantiations).
>>
>>
>> AFAIK, it is not common practice to add diagnostics on template
>> declarations as this will break SDKs.
>>
>
> Not sure I follow - any type in a header/library could be violating this
> rule, whether it's a class template or a straight class.
>
>
> By this I meant that we don’t issue diagnostics on template declarations
> in other situations.
>

Right, usually that's the right thing - for example, division by zero: we
want to catch all the extra cases of div by zero that might happen only in
specific instantiations of a template (eg: template<int n> void func() { x
/ n; } - if we used the template pattern we wouldn't see a div by zero, but
by using the instantiation we can see div by zero in func<0>). It just
happens to be in this instance it's the other way around - & it's the case
for a variety of other warnings too (unreachable code is the only other one
that comes to my mind... I'm somewhat sure there would be other cases).


> This will be the “first” and there
> may be templates that no one is using and this will break SDKs for those
> (something template writers may not be used to).
>
>
>
>> Furthermore, if template is not used
>> in practice, there is no point issuing this diagnostic.
>>
>
> Same would be true of non-template classes in headers, but we don't avoid
> those.
>
>
> Templates are special in that for whatever reasons, we skip diagnostics on
> them.
>
>
>
>> Can you point to similar situations where templates are iterated over
>> checking for method
>> inconsistencies or other types of diagnostics?
>>
>
> Not that I recall/off-hand.
>
>
> If we do none. This will be first and people may have strong opinion on
> the “firsts” :).
>
>
>
> I do recall Ted objecting to my desire to move unreachable-code warnings
> to act on template patterns rather than template instantiations  (& he
> didn't have numbers either, but the case is different - in that case there
> are many more function template definitions in headers that aren't
> necessarily instantiated (owing to the language need for the definitions to
> be available for instantiations) - unlike classes V class templates, which
> just appear in headers in a fairly similar way, not a particularly biased
> ratio one way or the other (except by library design preferences))
>
>
> I think similar situation is applicable to class templates which may not
> be instantiated. I just don’t know enough about usages etc. to make a
> strong argument one way
> or the other. So, other opinions are most welcome.
>

Yeah, I'd love to have some other voices here too

- Dave


> - Fariborz
>
>
> - David
>
>
>>
>> - Fariborz
>>
>>
>>
>>>
>>> - Fariborz
>>>
>>>
>>> - David
>>>
>>> On Wed, Feb 25, 2015 at 10:39 AM, jahanian <fjahanian at apple.com> wrote:
>>>
>>>>
>>>> This patch restricts issuing -Winconsistent-missing-override when
>>>> dealing with
>>>> class template with dependent bases and dependent methods.
>>>> Fixed pr22582 rdar://19917107.
>>>> Please review.
>>>>
>>>> - Fariborz
>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> cfe-commits mailing list
>>>> cfe-commits at cs.uiuc.edu
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>>
>>>>
>>>
>>>
>>
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150307/22aa13df/attachment.html>