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

[Reid Kleckner]

+def CXX11WarnOverrideMethod :
DiagGroup<"warn-missing-override-on-overriding-method">;

Anyone care to bikeshed the name a bit? This flag feels really verbose,
especially given that it gets a -W prefix.

Can we go with -Wmissing-override or -Winconsistent-missing-override to
reflect the fact that it's heuristically triggered based on use of C++11
override somewhere in the class hierarchy?

On Fri, Sep 26, 2014 at 4:15 PM, jahanian <fjahanian at apple.com> wrote:

>
> 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
>
>
>
>
> _______________________________________________
> 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/20140930/83e5de76/attachment.html>