> On Sep 26, 2014, at 3:03 PM, jahanian <[email protected]> wrote:
>
>
> On Sep 25, 2014, at 11:51 AM, Argyrios Kyrtzidis <[email protected]
> <mailto:[email protected]>> wrote:
>
>>
>>> On Sep 25, 2014, at 11:24 AM, Douglas Gregor <[email protected]
>>> <mailto:[email protected]>> 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?
- Doug
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
