CXX11 patch to warn if 'override' is missing on overriding virtual function

[jahanian]

On Sep 26, 2014, at 4:12 PM, Douglas Gregor <dgregor at apple.com> wrote:

> LGTM
> 
Thanks. I want to add FixIts before checking it in.

- Fariborz

>> On Sep 26, 2014, at 4:10 PM, jahanian <fjahanian at apple.com> wrote:
>> 
>> 
>> On Sep 26, 2014, at 3:37 PM, Douglas Gregor <dgregor at apple.com> wrote:
>> 
>>> 
>>>> On Sep 26, 2014, at 3:03 PM, jahanian <fjahanian at apple.com> wrote:
>>>> 
>>>> 
>>>> On Sep 25, 2014, at 11:51 AM, Argyrios Kyrtzidis <kyrtzidis at apple.com> wrote:
>>>> 
>>>>> 
>>>>>> On Sep 25, 2014, at 11:24 AM, Douglas Gregor <dgregor at apple.com> wrote:
>>>>>> 
>>>>>>  I’d feel a lot better if some part of the warning could be on by default. For example, if you’ve uttered “override” at least once in a class, it makes sense to warn-by-default about any other overrides in that class that weren’t marked as “override”, because you’re being locally inconsistent. Or maybe you can expand that heuristic out to a file-level granularity (which matches better for the null point constant warning) and still be on-by-default.
>>>>> 
>>>>> This seems like a great idea to me!
>>>>> For the 'override' I much prefer if it is class specific to make it less of a burden as an “always on” warning. We could have the checking done at the end of the class definition.
>>>>> 
>>>> 
>>>> Here is the patch. Warning is on by default. Number of new warnings on clang tests is greatly reduced but there are still some.
>>> 
>>> +def warn_function_marked_not_override_overriding : Warning <
>>> +  "%0 is not marked 'override' but overrides a member functions">,
>>> +  InGroup<CXX11WarnOverrideMethod>;
>>> 
>>> “a member functions” shouldn’t be plural. Also, perhaps we should turn this around:
>>> 
>>> 	“%0 overrides a member function but is not marked ‘override’”
>>> 
>>> because that puts the context of the problem before the problem.
>>> 
>>> +  if (HasMethodWithOverrideControl) {
>>> +    // At list one method has the 'override' control declared.
>>> +    // Diagnose all other overridden methods which do not have 'override' specified on them.
>>> +    for (auto *M : Record->methods())
>>> 
>>> “At list” -> “At least”.
>>> 
>>> Also, this means we’ll be taking two passes over the methods if any “override” is present, even though we won’t often warn here. How about extending this:
>>> 
>>> +      if (M->hasAttr<OverrideAttr>())
>>> +        HasMethodWithOverrideControl = true;
>>> 
>>> with
>>> 
>>> 	else if (M->begin_overridden_methods() != M->end_overridden_methods())
>>> 	  HasOverridingMethodWithoutOverrideControl = true;
>>> 
>>> and we only do this second pass when we know we’re going to warn, e.g., if HasMethodWithOverrideControl && HasOverridingMethodWithoutOverrideControl?
>> 
>> Thanks for quick review. Here is the updated patch.
>> 	
>> <override-patch.txt>
>> 
>> - Fariborz
>>> 
>>> 	- Doug
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140926/f0b97a3f/attachment.html>