CXX11 patch to warn if 'override' is missing on overriding virtual function
Reid Kleckner
rnk at google.com
Tue Sep 30 14:58:44 PDT 2014
+cc clang-tidy people, since they rolled this kind of stuff out already.
Index: test/Parser/cxx0x-in-cxx98.cpp
===================================================================
--- test/Parser/cxx0x-in-cxx98.cpp (revision 218519)
+++ test/Parser/cxx0x-in-cxx98.cpp (working copy)
@@ -10,11 +10,12 @@
struct B {
virtual void f();
- virtual void g();
+ virtual void g(); // expected-note {{overridden virtual function is
here}}
};
struct D final : B { // expected-warning {{'final' keyword is a C++11
extension}}
virtual void f() override; // expected-warning {{'override' keyword is a
C++11 extension}}
- virtual void g() final; // expected-warning {{'final' keyword is a C++11
extension}}
+ virtual void g() final; // expected-warning {{'final' keyword is a C++11
extension}} \
+ // expected-warning {{'g' overrides a member function but is not marked
'override'}}
};
void NewBracedInitList() {
Should we really be suggesting that users add C++11 features like override
when -Wcxx98-compat is on? That seems undesirable.
Index: test/Parser/cxx0x-decl.cpp
===================================================================
--- test/Parser/cxx0x-decl.cpp (revision 218519)
+++ test/Parser/cxx0x-decl.cpp (working copy)
@@ -83,13 +83,13 @@
namespace FinalOverride {
struct Base {
- virtual void *f();
+ virtual void *f(); // expected-note {{overridden virtual function is
here}}
virtual void *g();
virtual void *h();
virtual void *i();
};
struct Derived : Base {
- virtual auto f() -> void *final;
+ virtual auto f() -> void *final; // expected-warning {{'f' overrides a
member function but is not marked 'override'}}
virtual auto g() -> void *override;
virtual auto h() -> void *final override;
virtual auto i() -> void *override final;
Why make this suggestion? 'override' is redundant in the presence of
'final', assuming that we warn on final virtual methods that don't override
anything, which we probably should.
Index: test/SemaCXX/ms-interface.cpp
===================================================================
--- test/SemaCXX/ms-interface.cpp (revision 218519)
+++ test/SemaCXX/ms-interface.cpp (working copy)
@@ -12,7 +12,7 @@
operator int();
// expected-error at +1 {{nested class I1::(anonymous) is not permitted
within an interface type}}
struct { int a; };
- void fn2() {
+ void fn2() { // expected-note {{overridden virtual function is here}}
struct A { }; // should be ignored: not a nested class
}
protected: // expected-error {{interface types cannot specify 'protected'
access}}
@@ -44,7 +44,7 @@
__interface I4 : I1, I2 {
void fn1() const override;
// expected-error at +1 {{'final' keyword not permitted with interface
types}}
- void fn2() final;
+ void fn2() final; // expected-warning {{'fn2' overrides a member
function but is not marked 'override'}}
};
// expected-error at +1 {{interface type cannot inherit from non-public
'interface I1'}}
ditto
On Tue, Sep 30, 2014 at 2:45 PM, Reid Kleckner <rnk at google.com> wrote:
> +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/a856a6a1/attachment.html>
More information about the cfe-commits
mailing list