On Sep 26, 2014, at 3:37 PM, Douglas Gregor <[email protected]> wrote:


On Sep 26, 2014, at 3:03 PM, jahanian <[email protected]> wrote:


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.

+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.
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 overrides a member function but is not marked 'override'">,
+  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,18 @@
     }
   }
 
+  bool HasMethodWithOverrideControl = false,
+       HasOverridingMethodWithoutOverrideControl = 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;
+      else if (M->begin_overridden_methods() != M->end_overridden_methods())
+        HasOverridingMethodWithoutOverrideControl = true;
       // Check whether the explicitly-defaulted special members are valid.
       if (!M->isInvalidDecl() && M->isExplicitlyDefaulted())
         CheckExplicitlyDefaultedSpecialMember(M);
@@ -4675,6 +4701,14 @@
     }
   }
 
+  if (HasMethodWithOverrideControl
+      && HasOverridingMethodWithoutOverrideControl) {
+    // At least 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' overrides a member function 
but is not marked 'override'}}
     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' overrides a member function but 
is not marked 'override'}}
   };
   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' overrides a member function but is 
not marked 'override'}}
       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' overrides a member function but is 
not marked 'override'}}
   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' 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;
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() {
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' 
overrides a member function but is not marked 'override'}}
 
   // 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' 
overrides a member function but is not marked 'override'}}
   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' overrides a member function but is 
not marked 'override'}}
+};
+
+struct C : B 
+{
+    virtual void g() = 0;  // expected-warning {{'g' overrides a member 
function but is not marked 'override'}}
+    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' overrides a member function 
but is not marked 'override'}}
 };
 
 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' overrides a member function 
but is not marked 'override'}}
 };
 
 // expected-error@+1 {{interface type cannot inherit from non-public 
'interface I1'}}

- Fariborz

- Doug



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

Reply via email to