From: Anders Carlsson <andersca@mac.com>
Date: Thu, 14 May 2009 01:09:04 +0000 (+0000)
Subject: Add return type checking for overriding virtual functions. We currently don't check... 
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=d7ba27dc562e1837703fa8696531c1f21a5fdb36;p=clang

Add return type checking for overriding virtual functions. We currently don't check covariance but that's next.

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

diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td
index 4d018ae01b..f23ea2a10e 100644
--- a/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/include/clang/Basic/DiagnosticSemaKinds.td
@@ -301,6 +301,12 @@ def err_implicit_object_parameter_init : Error<
   "cannot initialize object parameter of type %0 with an expression "
   "of type %1">;
 
+def err_different_return_type_for_overriding_virtual_function : Error<
+  "virtual function %0 has a different return type (%1) than the "
+  "function it overrides (which has return type %2)">;
+def note_overridden_virtual_function : Note<
+  "overridden virtual function is here">;
+
 // C++ constructors
 def err_constructor_cannot_be : Error<"constructor cannot be declared '%0'">;
 def err_invalid_qualified_constructor : Error<
diff --git a/lib/Sema/Sema.h b/lib/Sema/Sema.h
index bd01ec9e51..5b263b7d92 100644
--- a/lib/Sema/Sema.h
+++ b/lib/Sema/Sema.h
@@ -1780,6 +1780,12 @@ public:
                                     DeclarationName Name);
   
   std::string getAmbiguousPathsDisplayString(BasePaths &Paths);
+  
+  /// CheckReturnTypeCovariance - Checks whether two types are covariant, 
+  /// according to C++ [class.virtual]p5.
+  bool CheckOverridingFunctionReturnType(const CXXMethodDecl *New, 
+                                         const CXXMethodDecl *Old);
+  
 
   //===--------------------------------------------------------------------===//
   // C++ Access Control
diff --git a/lib/Sema/SemaDecl.cpp b/lib/Sema/SemaDecl.cpp
index e27517e689..a2f7af1d8b 100644
--- a/lib/Sema/SemaDecl.cpp
+++ b/lib/Sema/SemaDecl.cpp
@@ -12,6 +12,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "Sema.h"
+#include "SemaInherit.h"
 #include "clang/AST/APValue.h"
 #include "clang/AST/ASTConsumer.h"
 #include "clang/AST/ASTContext.h"
@@ -2113,10 +2114,6 @@ Sema::ActOnFunctionDeclarator(Scope* S, Declarator& D, DeclContext* DC,
   //   nonstatic class member functions that appear within a
   //   member-specification of a class declaration; see 10.3.
   //
-  // FIXME: Checking the 'virtual' specifier is not sufficient. A
-  // function is also virtual if it overrides an already virtual
-  // function. This is important to do here because it's part of the
-  // declaration.
   if (isVirtual && !NewFD->isInvalidDecl()) {
     if (!isVirtualOkay) {
        Diag(D.getDeclSpec().getVirtualSpecLoc(), 
@@ -2137,6 +2134,36 @@ Sema::ActOnFunctionDeclarator(Scope* S, Declarator& D, DeclContext* DC,
     }
   }
 
+  if (CXXMethodDecl *NewMD = dyn_cast<CXXMethodDecl>(NewFD)) {
+    // Look for virtual methods in base classes that this method might override.
+
+    BasePaths Paths;
+    // FIXME: This will not include hidden member functions.
+    if (LookupInBases(cast<CXXRecordDecl>(DC), 
+                      MemberLookupCriteria(Name, LookupMemberName, 
+                                           // FIXME: Shouldn't IDNS_Member be
+                                           // enough here?
+                                           Decl::IDNS_Member | 
+                                           Decl::IDNS_Ordinary), Paths)) {
+      for (BasePaths::decl_iterator I = Paths.found_decls_begin(), 
+           E = Paths.found_decls_end(); I != E; ++I) {
+        if (CXXMethodDecl *OldMD = dyn_cast<CXXMethodDecl>(*I)) {
+          OverloadedFunctionDecl::function_iterator MatchedDecl;
+          // FIXME: Is this OK? Should it be done by LookupInBases?
+          if (IsOverload(NewMD, OldMD, MatchedDecl))
+            continue;
+         
+          if (!CheckOverridingFunctionReturnType(NewMD, OldMD)) {
+            // FIXME: Add OldMD to the list of methods NewMD overrides.
+          }
+          
+        }
+      }
+                        
+    }
+    
+  }
+  
   if (SC == FunctionDecl::Static && isa<CXXMethodDecl>(NewFD) && 
       !CurContext->isRecord()) {
     // C++ [class.static]p1:
diff --git a/lib/Sema/SemaDeclCXX.cpp b/lib/Sema/SemaDeclCXX.cpp
index 726080b7ff..bdd3cc2be1 100644
--- a/lib/Sema/SemaDeclCXX.cpp
+++ b/lib/Sema/SemaDeclCXX.cpp
@@ -2688,3 +2688,25 @@ void Sema::DiagnoseReturnInConstructorExceptionHandler(CXXTryStmt *TryBlock) {
     SearchForReturnInStmt(*this, Handler);
   }
 }
+
+bool Sema::CheckOverridingFunctionReturnType(const CXXMethodDecl *New, 
+                                             const CXXMethodDecl *Old) {
+  QualType NewTy = New->getType()->getAsFunctionType()->getResultType();
+  QualType OldTy = Old->getType()->getAsFunctionType()->getResultType();
+
+  QualType CNewTy = Context.getCanonicalType(NewTy);
+  QualType COldTy = Context.getCanonicalType(OldTy);
+
+  if (CNewTy == COldTy && 
+      CNewTy.getCVRQualifiers() == COldTy.getCVRQualifiers())
+    return false;
+  
+  // FIXME: Check covariance.
+
+  Diag(New->getLocation(), 
+       diag::err_different_return_type_for_overriding_virtual_function)
+    << New->getDeclName() << NewTy << OldTy;
+  Diag(Old->getLocation(), diag::note_overridden_virtual_function);
+       
+  return true;
+}
diff --git a/test/SemaCXX/virtual-override.cpp b/test/SemaCXX/virtual-override.cpp
new file mode 100644
index 0000000000..c1b95ccbdf
--- /dev/null
+++ b/test/SemaCXX/virtual-override.cpp
@@ -0,0 +1,13 @@
+// RUN: clang-cc -fsyntax-only -verify %s
+
+namespace T1 {
+
+class A {
+  virtual int f(); // expected-note{{overridden virtual function is here}}
+};
+
+class B : A {
+  virtual void f(); // expected-error{{virtual function 'f' has a different return type ('void') than the function it overrides (which has return type 'int')}}
+};
+
+}