On Sep 25, 2014, at 11:51 AM, Argyrios Kyrtzidis <[email protected]> wrote:


On Sep 25, 2014, at 11:24 AM, Douglas Gregor <[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.

Index: include/clang/Basic/DiagnosticGroups.td
===================================================================
--- include/clang/Basic/DiagnosticGroups.td     (revision 218519)
+++ include/clang/Basic/DiagnosticGroups.td     (working copy)
@@ -146,6 +146,8 @@
 
 def CXX11Narrowing : DiagGroup<"c++11-narrowing">;
 
+def CXX11WarnOverrideMethod : 
DiagGroup<"warn-missing-override-on-overriding-method">;
+
 // Original name of this warning in Clang
 def : DiagGroup<"c++0x-narrowing", [CXX11Narrowing]>;
 
Index: include/clang/Basic/DiagnosticSemaKinds.td
===================================================================
--- include/clang/Basic/DiagnosticSemaKinds.td  (revision 218519)
+++ include/clang/Basic/DiagnosticSemaKinds.td  (working copy)
@@ -1688,6 +1688,9 @@
   "%select{function|functions}1">;
 def err_function_marked_override_not_overriding : Error<
   "%0 marked 'override' but does not override any member functions">;
+def warn_function_marked_not_override_overriding : Warning <
+  "%0 is not marked 'override' but overrides a member functions">,
+  InGroup<CXX11WarnOverrideMethod>;
 def err_class_marked_final_used_as_base : Error<
   "base %0 is marked '%select{final|sealed}1'">;
 def warn_abstract_final_class : Warning<
Index: include/clang/Sema/Sema.h
===================================================================
--- include/clang/Sema/Sema.h   (revision 218519)
+++ include/clang/Sema/Sema.h   (working copy)
@@ -5084,6 +5084,10 @@
 
   /// CheckOverrideControl - Check C++11 override control semantics.
   void CheckOverrideControl(NamedDecl *D);
+    
+  /// DiagnoseAbsenseOfOverrideControl - Diagnose if override control was
+  /// not used in the; otherwise, overriding method.
+  void DiagnoseAbsenseOfOverrideControl(NamedDecl *D);
 
   /// CheckForFunctionMarkedFinal - Checks whether a virtual member function
   /// overrides a virtual member function marked 'final', according to
Index: lib/Sema/SemaDeclCXX.cpp
===================================================================
--- lib/Sema/SemaDeclCXX.cpp    (revision 218519)
+++ lib/Sema/SemaDeclCXX.cpp    (working copy)
@@ -1895,6 +1895,27 @@
       << MD->getDeclName();
 }
 
+void Sema::DiagnoseAbsenseOfOverrideControl(NamedDecl *D) {
+  if (D->isInvalidDecl())
+    return;
+  CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(D);
+  if (!MD || MD->isImplicit())
+    return;
+
+  bool HasOverriddenMethods =
+    MD->begin_overridden_methods() != MD->end_overridden_methods();
+  if (HasOverriddenMethods) {
+    Diag(MD->getLocation(), diag::warn_function_marked_not_override_overriding)
+      << MD->getDeclName();
+    for (CXXMethodDecl::method_iterator I = MD->begin_overridden_methods(),
+         E = MD->end_overridden_methods(); I != E; ++I) {
+      const CXXMethodDecl *OMD = *I;
+      Diag(OMD->getLocation(), diag::note_overridden_virtual_function);
+      break;
+    }
+  }
+}
+
 /// CheckIfOverriddenFunctionIsMarkedFinal - Checks whether a virtual member
 /// function overrides a virtual member function marked 'final', according to
 /// C++11 [class.virtual]p4.
@@ -4650,13 +4671,15 @@
     }
   }
 
+  bool HasMethodWithOverrideControl = false;
   if (!Record->isDependentType()) {
     for (auto *M : Record->methods()) {
       // See if a method overloads virtual methods in a base
       // class without overriding any.
       if (!M->isStatic())
         DiagnoseHiddenVirtualMethods(M);
-
+      if (M->hasAttr<OverrideAttr>())
+        HasMethodWithOverrideControl = true;
       // Check whether the explicitly-defaulted special members are valid.
       if (!M->isInvalidDecl() && M->isExplicitlyDefaulted())
         CheckExplicitlyDefaultedSpecialMember(M);
@@ -4675,6 +4698,13 @@
     }
   }
 
+  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())
+      if (!M->hasAttr<OverrideAttr>())
+        DiagnoseAbsenseOfOverrideControl(M);
+  }
   // C++11 [dcl.constexpr]p8: A constexpr specifier for a non-static member
   // function that is not a constructor declares that member function to be
   // const. [...] The class of which that function is a member shall be
Index: test/CXX/class.derived/class.virtual/p3-0x.cpp
===================================================================
--- test/CXX/class.derived/class.virtual/p3-0x.cpp      (revision 218519)
+++ test/CXX/class.derived/class.virtual/p3-0x.cpp      (working copy)
@@ -61,7 +61,7 @@
 namespace PR13499 {
   struct X {
     virtual void f();
-    virtual void h();
+    virtual void h(); // expected-note 2 {{overridden virtual function is 
here}}
   };
   template<typename T> struct A : X {
     void f() override;
@@ -83,7 +83,7 @@
   template<typename...T> struct E : X {
     void f(T...) override;
     void g(T...) override; // expected-error {{only virtual member functions 
can be marked 'override'}}
-    void h(T...) final;
+    void h(T...) final; // expected-warning {{'h' is not marked 'override' but 
overrides a member functions}}
     void i(T...) final; // expected-error {{only virtual member functions can 
be marked 'final'}}
   };
   // FIXME: Diagnose these in the template definition, not in the 
instantiation.
@@ -91,13 +91,13 @@
 
   template<typename T> struct Y : T {
     void f() override;
-    void h() final;
+    void h() final; // expected-warning {{'h' is not marked 'override' but 
overrides a member functions}}
   };
   template<typename T> struct Z : T {
     void g() override; // expected-error {{only virtual member functions can 
be marked 'override'}}
     void i() final; // expected-error {{only virtual member functions can be 
marked 'final'}}
   };
-  Y<X> y;
+  Y<X> y; // expected-note {{in instantiation of}}
   Z<X> z; // expected-note {{in instantiation of}}
 }
 
Index: test/FixIt/fixit-cxx0x.cpp
===================================================================
--- test/FixIt/fixit-cxx0x.cpp  (revision 218519)
+++ test/FixIt/fixit-cxx0x.cpp  (working copy)
@@ -24,7 +24,7 @@
   int o;
 
   struct Base {
-    virtual void f2(), f3();
+    virtual void f2(), f3(); // expected-note {{overridden virtual function is 
here}}
   };
   struct MemberDeclarator : Base {
     int k : 4,
@@ -33,7 +33,7 @@
     char c, // expected-error {{expected ';' at end of declaration}}
     typedef void F(), // expected-error {{expected ';' at end of declaration}}
     F f1,
-      f2 final,
+      f2 final, // expected-warning {{'f2' is not marked 'override' but 
overrides a member functions}}
       f3 override, // expected-error {{expected ';' at end of declaration}}
   };
 }
Index: test/Parser/MicrosoftExtensions.cpp
===================================================================
--- test/Parser/MicrosoftExtensions.cpp (revision 218519)
+++ test/Parser/MicrosoftExtensions.cpp (working copy)
@@ -208,12 +208,12 @@
 
 __interface MicrosoftInterface;
 __interface MicrosoftInterface {
-   void foo1() = 0;
+   void foo1() = 0; // expected-note {{overridden virtual function is here}}
    virtual void foo2() = 0;
 };
 
 __interface MicrosoftDerivedInterface : public MicrosoftInterface {
-  void foo1();
+  void foo1(); // expected-warning {{'foo1' is not marked 'override' but 
overrides a member functions}}
   void foo2() override;
   void foo3();
 };
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' is not marked 
'override' but overrides a member functions}}
     virtual auto g() -> void *override;
     virtual auto h() -> void *final override;
     virtual auto i() -> void *override final;
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' is not marked 'override' 
but overrides a member functions}}
 };
 
 void NewBracedInitList() {
Index: test/SemaCXX/MicrosoftExtensions.cpp
===================================================================
--- test/SemaCXX/MicrosoftExtensions.cpp        (revision 218519)
+++ test/SemaCXX/MicrosoftExtensions.cpp        (working copy)
@@ -372,14 +372,14 @@
 
   // expected-note@+2 {{overridden virtual function is here}}
   // expected-warning@+1 {{'sealed' keyword is a Microsoft extension}}
-  virtual void SealedFunction() sealed;
+  virtual void SealedFunction() sealed; // expected-note {{overridden virtual 
function is here}}
 };
 
 // expected-note@+2 {{'SealedType' declared here}}
 // expected-warning@+1 {{'sealed' keyword is a Microsoft extension}}
 struct SealedType sealed : SomeBase {
   // expected-error@+1 {{declaration of 'SealedFunction' overrides a 'sealed' 
function}}
-  virtual void SealedFunction();
+  virtual void SealedFunction(); // expected-warning {{'SealedFunction' is not 
marked 'override' but overrides a member functions}}
 
   // expected-warning@+1 {{'override' keyword is a C++11 extension}}
   virtual void OverrideMe() override;
Index: test/SemaCXX/attr-gnu.cpp
===================================================================
--- test/SemaCXX/attr-gnu.cpp   (revision 218519)
+++ test/SemaCXX/attr-gnu.cpp   (working copy)
@@ -15,14 +15,15 @@
 namespace {
 class B {
 public:
-  virtual void test() {}
+  virtual void test() {} // expected-note {{overridden virtual function is 
here}}
   virtual void test2() {}
   virtual void test3() {}
 };
 
 class D : public B {
 public:
-  void test() __attribute__((deprecated)) final {} // expected-warning {{GCC 
does not allow an attribute in this position on a function declaration}}
+  void test() __attribute__((deprecated)) final {} // expected-warning {{GCC 
does not allow an attribute in this position on a function declaration}} \
+                                                  // expected-warning {{'test' 
is not marked 'override' but overrides a member functions}}
   void test2() [[]] override {} // Ok
   void test3() __attribute__((cf_unknown_transfer)) override {} // Ok, not 
known to GCC.
 };
Index: test/SemaCXX/cxx11-warn-missing-override.cpp
===================================================================
--- test/SemaCXX/cxx11-warn-missing-override.cpp        (revision 0)
+++ test/SemaCXX/cxx11-warn-missing-override.cpp        (working copy)
@@ -0,0 +1,21 @@
+// RUN: %clang_cc1 -fsyntax-only -Wwarn-missing-override-on-overriding-method 
-verify -std=c++11 %s
+struct A
+{
+    virtual void foo();
+    virtual void bar(); // expected-note {{overridden virtual function is 
here}}
+    virtual void gorf() {}
+    virtual void g() = 0; // expected-note {{overridden virtual function is 
here}}
+};
+ 
+struct B : A
+{
+    void foo() override;
+    void bar(); // expected-warning {{'bar' is not marked 'override' but 
overrides a member functions}}
+};
+
+struct C : B 
+{
+    virtual void g() = 0;  // expected-warning {{'g' is not marked 'override' 
but overrides a member functions}}
+    virtual void gorf() override {}
+};
+
Index: test/SemaCXX/cxx98-compat.cpp
===================================================================
--- test/SemaCXX/cxx98-compat.cpp       (revision 218519)
+++ test/SemaCXX/cxx98-compat.cpp       (working copy)
@@ -120,11 +120,12 @@
 
 struct OverrideControlBase {
   virtual void f();
-  virtual void g();
+  virtual void g(); // expected-note {{overridden virtual function is here}}
 };
 struct OverrideControl final : OverrideControlBase { // expected-warning 
{{'final' keyword is incompatible with C++98}}
   virtual void f() override; // expected-warning {{'override' keyword is 
incompatible with C++98}}
-  virtual void g() final; // expected-warning {{'final' keyword is 
incompatible with C++98}}
+  virtual void g() final; // expected-warning {{'final' keyword is 
incompatible with C++98}} \
+                         // expected-warning {{'g' is not marked 'override' 
but overrides a member functions}}
 };
 
 using AliasDecl = int; // expected-warning {{alias declarations are 
incompatible with C++98}}
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@+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@+1 {{'final' keyword not permitted with interface types}}
-  void fn2() final;
+  void fn2() final; // expected-warning {{'fn2' is not marked 'override' but 
overrides a member functions}}
 };
 
 // expected-error@+1 {{interface type cannot inherit from non-public 
'interface I1'}}

- Fariborz

_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Reply via email to