Improve error for "override" + non-virtual func.

Consider something like the following:

struct X {
  virtual void foo(float x);
};
struct Y : X {
  void foo(double x) override;
};

The error is almost certainly that Y::foo() has the wrong signature,
rather than incorrect usage of the override keyword.  This patch
adds an appropriate diagnostic for that case.

Fixes <rdar://problem/14785106>.

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@190109 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td
index 2e0a01a90df5a6126020b138d55d7c76e982581b..ddf529e80514d6d23952cab4aa1975de83453801 100644 (file)
--- a/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/include/clang/Basic/DiagnosticSemaKinds.td
@@ -1538,6 +1538,9 @@ def err_auto_fn_virtual : Error<
 // C++11 override control
 def override_keyword_only_allowed_on_virtual_member_functions : Error<
   "only virtual member functions can be marked '%0'">;
+def override_keyword_hides_virtual_member_function : Error<
+  "non-virtual member function marked '%0' hides virtual member "
+  "%select{function|functions}1">;
 def err_function_marked_override_not_overriding : Error<
   "%0 marked 'override' but does not override any member functions">;
 def err_class_marked_final_used_as_base : Error<

diff --git a/include/clang/Sema/Sema.h b/include/clang/Sema/Sema.h
index 553a86faff68dd50329245168cb3a930aa646d6a..e46899be592255acbd538d33280ae4afb81e29cd 100644 (file)
--- a/include/clang/Sema/Sema.h
+++ b/include/clang/Sema/Sema.h
@@ -1490,7 +1490,11 @@ public:
   bool CheckConstexprFunctionDecl(const FunctionDecl *FD);
   bool CheckConstexprFunctionBody(const FunctionDecl *FD, Stmt *Body);
 
-  void DiagnoseHiddenVirtualMethods(CXXRecordDecl *DC, CXXMethodDecl *MD);
+  void DiagnoseHiddenVirtualMethods(CXXMethodDecl *MD);
+  void FindHiddenVirtualMethods(CXXMethodDecl *MD,
+                          SmallVectorImpl<CXXMethodDecl*> &OverloadedMethods);
+  void NoteHiddenVirtualMethods(CXXMethodDecl *MD,
+                          SmallVectorImpl<CXXMethodDecl*> &OverloadedMethods);
   // Returns true if the function declaration is a redeclaration
   bool CheckFunctionDeclaration(Scope *S,
                                 FunctionDecl *NewFD, LookupResult &Previous,
@@ -4807,7 +4811,7 @@ public:
   bool CheckPureMethod(CXXMethodDecl *Method, SourceRange InitRange);
 
   /// CheckOverrideControl - Check C++11 override control semantics.
-  void CheckOverrideControl(Decl *D);
+  void CheckOverrideControl(NamedDecl *D);
 
   /// CheckForFunctionMarkedFinal - Checks whether a virtual member function
   /// overrides a virtual member function marked 'final', according to

diff --git a/lib/Sema/SemaDeclCXX.cpp b/lib/Sema/SemaDeclCXX.cpp
index 01f3b7cb491c1633f86fab3306af97488a74ba15..a4ed404cc9c54c131be723f52444f13a84590bff 100644 (file)
--- a/lib/Sema/SemaDeclCXX.cpp
+++ b/lib/Sema/SemaDeclCXX.cpp
@@ -1700,37 +1700,61 @@ bool Sema::ActOnAccessSpecifier(AccessSpecifier Access,
 }
 
 /// CheckOverrideControl - Check C++11 override control semantics.
-void Sema::CheckOverrideControl(Decl *D) {
+void Sema::CheckOverrideControl(NamedDecl *D) {
   if (D->isInvalidDecl())
     return;
 
-  const CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(D);
+  // We only care about "override" and "final" declarations.
+  if (!D->hasAttr<OverrideAttr>() && !D->hasAttr<FinalAttr>())
+    return;
 
-  // Do we know which functions this declaration might be overriding?
-  bool OverridesAreKnown = !MD ||
-      (!MD->getParent()->hasAnyDependentBases() &&
-       !MD->getType()->isDependentType());
+  CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(D);
 
-  if (!MD || !MD->isVirtual()) {
-    if (OverridesAreKnown) {
+  // We can't check dependent instance methods.
+  if (MD && MD->isInstance() &&
+      (MD->getParent()->hasAnyDependentBases() ||
+       MD->getType()->isDependentType()))
+    return;
+
+  if (MD && !MD->isVirtual()) {
+    // If we have a non-virtual method, check if if hides a virtual method.
+    // (In that case, it's most likely the method has the wrong type.)
+    SmallVector<CXXMethodDecl *, 8> OverloadedMethods;
+    FindHiddenVirtualMethods(MD, OverloadedMethods);
+
+    if (!OverloadedMethods.empty()) {
       if (OverrideAttr *OA = D->getAttr<OverrideAttr>()) {
         Diag(OA->getLocation(),
-             diag::override_keyword_only_allowed_on_virtual_member_functions)
-          << "override" << FixItHint::CreateRemoval(OA->getLocation());
-        D->dropAttr<OverrideAttr>();
-      }
-      if (FinalAttr *FA = D->getAttr<FinalAttr>()) {
+             diag::override_keyword_hides_virtual_member_function)
+          << "override" << (OverloadedMethods.size() > 1);
+      } else if (FinalAttr *FA = D->getAttr<FinalAttr>()) {
         Diag(FA->getLocation(),
-             diag::override_keyword_only_allowed_on_virtual_member_functions)
-          << "final" << FixItHint::CreateRemoval(FA->getLocation());
-        D->dropAttr<FinalAttr>();
+             diag::override_keyword_hides_virtual_member_function)
+            << "final" << (OverloadedMethods.size() > 1);
       }
+      NoteHiddenVirtualMethods(MD, OverloadedMethods);
+      MD->setInvalidDecl();
+      return;
     }
-    return;
+    // Fall through into the general case diagnostic.
+    // FIXME: We might want to attempt typo correction here.
   }
 
-  if (!OverridesAreKnown)
+  if (!MD || !MD->isVirtual()) {
+    if (OverrideAttr *OA = D->getAttr<OverrideAttr>()) {
+      Diag(OA->getLocation(),
+           diag::override_keyword_only_allowed_on_virtual_member_functions)
+        << "override" << FixItHint::CreateRemoval(OA->getLocation());
+      D->dropAttr<OverrideAttr>();
+    }
+    if (FinalAttr *FA = D->getAttr<FinalAttr>()) {
+      Diag(FA->getLocation(),
+           diag::override_keyword_only_allowed_on_virtual_member_functions)
+        << "final" << FixItHint::CreateRemoval(FA->getLocation());
+      D->dropAttr<FinalAttr>();
+    }
     return;
+  }
 
   // C++11 [class.virtual]p5:
   //   If a virtual function is marked with the virt-specifier override and
@@ -4222,7 +4246,7 @@ void Sema::CheckCompletedCXXClass(CXXRecordDecl *Record) {
       // See if a method overloads virtual methods in a base
       // class without overriding any.
       if (!M->isStatic())
-        DiagnoseHiddenVirtualMethods(Record, *M);
+        DiagnoseHiddenVirtualMethods(*M);
 
       // Check whether the explicitly-defaulted special members are valid.
       if (!M->isInvalidDecl() && M->isExplicitlyDefaulted())
@@ -5604,12 +5628,10 @@ static void AddMostOverridenMethods(const CXXMethodDecl *MD,
     AddMostOverridenMethods(*I, Methods);
 }
 
-/// \brief See if a method overloads virtual methods in a base class without
+/// \brief Check if a method overloads virtual methods in a base class without
 /// overriding any.
-void Sema::DiagnoseHiddenVirtualMethods(CXXRecordDecl *DC, CXXMethodDecl *MD) {
-  if (Diags.getDiagnosticLevel(diag::warn_overloaded_virtual,
-                               MD->getLocation()) == DiagnosticsEngine::Ignored)
-    return;
+void Sema::FindHiddenVirtualMethods(CXXMethodDecl *MD,
+                          SmallVectorImpl<CXXMethodDecl*> &OverloadedMethods) {
   if (!MD->getDeclName().isIdentifier())
     return;
 
@@ -5622,6 +5644,7 @@ void Sema::DiagnoseHiddenVirtualMethods(CXXRecordDecl *DC, CXXMethodDecl *MD) {
 
   // Keep the base methods that were overriden or introduced in the subclass
   // by 'using' in a set. A base method not in this set is hidden.
+  CXXRecordDecl *DC = MD->getParent();
   DeclContext::lookup_result R = DC->lookup(MD->getDeclName());
   for (DeclContext::lookup_iterator I = R.begin(), E = R.end(); I != E; ++I) {
     NamedDecl *ND = *I;
@@ -5631,18 +5654,38 @@ void Sema::DiagnoseHiddenVirtualMethods(CXXRecordDecl *DC, CXXMethodDecl *MD) {
       AddMostOverridenMethods(MD, Data.OverridenAndUsingBaseMethods);
   }
 
-  if (DC->lookupInBases(&FindHiddenVirtualMethod, &Data, Paths) &&
-      !Data.OverloadedMethods.empty()) {
+  if (DC->lookupInBases(&FindHiddenVirtualMethod, &Data, Paths))
+    OverloadedMethods = Data.OverloadedMethods;
+}
+
+void Sema::NoteHiddenVirtualMethods(CXXMethodDecl *MD,
+                          SmallVectorImpl<CXXMethodDecl*> &OverloadedMethods) {
+  for (unsigned i = 0, e = OverloadedMethods.size(); i != e; ++i) {
+    CXXMethodDecl *overloadedMD = OverloadedMethods[i];
+    PartialDiagnostic PD = PDiag(
+         diag::note_hidden_overloaded_virtual_declared_here) << overloadedMD;
+    HandleFunctionTypeMismatch(PD, MD->getType(), overloadedMD->getType());
+    Diag(overloadedMD->getLocation(), PD);
+  }
+}
+
+/// \brief Diagnose methods which overload virtual methods in a base class
+/// without overriding any.
+void Sema::DiagnoseHiddenVirtualMethods(CXXMethodDecl *MD) {
+  if (MD->isInvalidDecl())
+    return;
+
+  if (Diags.getDiagnosticLevel(diag::warn_overloaded_virtual,
+                               MD->getLocation()) == DiagnosticsEngine::Ignored)
+    return;
+
+  SmallVector<CXXMethodDecl *, 8> OverloadedMethods;
+  FindHiddenVirtualMethods(MD, OverloadedMethods);
+  if (!OverloadedMethods.empty()) {
     Diag(MD->getLocation(), diag::warn_overloaded_virtual)
-      << MD << (Data.OverloadedMethods.size() > 1);
+      << MD << (OverloadedMethods.size() > 1);
 
-    for (unsigned i = 0, e = Data.OverloadedMethods.size(); i != e; ++i) {
-      CXXMethodDecl *overloadedMD = Data.OverloadedMethods[i];
-      PartialDiagnostic PD = PDiag(
-           diag::note_hidden_overloaded_virtual_declared_here) << overloadedMD;
-      HandleFunctionTypeMismatch(PD, MD->getType(), overloadedMD->getType());
-      Diag(overloadedMD->getLocation(), PD);
-    }
+    NoteHiddenVirtualMethods(MD, OverloadedMethods);
   }
 }
 

diff --git a/test/CXX/class.derived/class.virtual/p3-0x.cpp b/test/CXX/class.derived/class.virtual/p3-0x.cpp
index 6a02a86731865b267c564e626d3b0f37412e70f8..41a5954bf8638e83009017759c4bfdc411e350d1 100644 (file)
--- a/test/CXX/class.derived/class.virtual/p3-0x.cpp
+++ b/test/CXX/class.derived/class.virtual/p3-0x.cpp
@@ -130,3 +130,23 @@ namespace MemberOfUnknownSpecialization {
   // expected-note@+1 {{in instantiation of}}
   A<double>::C c3;
 }
+
+namespace DiagnosticsQOI {
+  struct X {
+    virtual ~X();
+    virtual void foo(int x); // expected-note {{hidden overloaded virtual function}}
+    virtual void bar(int x); // expected-note 2 {{hidden overloaded virtual function}}
+    virtual void bar(float x); // expected-note 2 {{hidden overloaded virtual function}}
+  };
+
+  struct Y : X {
+    void foo(int x, int y) override; // expected-error {{non-virtual member function marked 'override' hides virtual member function}}
+    void bar(double) override; // expected-error {{non-virtual member function marked 'override' hides virtual member functions}}
+    void bar(long double) final; // expected-error {{non-virtual member function marked 'final' hides virtual member functions}}
+  };
+  
+  template<typename T>
+  struct Z : T {
+    static void foo() override; // expected-error {{only virtual member functions can be marked 'override'}}
+  };
+}