loplugin:unnecessaryoverride

Change-Id: I08c55a3023ec2e8990098eeb60e91cd18556e7ae
Reviewed-on: https://gerrit.libreoffice.org/29656
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
Tested-by: Noel Grandin <noel.grandin@collabora.co.uk>

1 files changed, 79 insertions, 45 deletions

diff --git a/compilerplugins/clang/unnecessaryoverride.cxx b/compilerplugins/clang/unnecessaryoverride.cxx
index d77b84f6037c..b031c50b7b87 100644
--- a/compilerplugins/clang/unnecessaryoverride.cxx
+++ b/compilerplugins/clang/unnecessaryoverride.cxx
@@ -39,6 +39,19 @@ public:
              return;
         if (fn == SRCDIR "/forms/source/component/Time.cxx")
             return;
+        if (fn == SRCDIR "/svx/source/dialog/hyperdlg.cxx")
+            return;
+        if (fn == SRCDIR "/svx/source/dialog/rubydialog.cxx")
+            return;
+        if (fn.startswith(SRCDIR "/canvas"))
+            return;
+        if (fn == SRCDIR "/sc/source/ui/view/spelldialog.cxx")
+            return;
+        if (fn == SRCDIR "/sd/source/ui/dlg/SpellDialogChildWindow.cxx")
+            return;
+        // HAVE_ODBC_ADMINISTRATION
+        if (fn == SRCDIR "/dbaccess/source/ui/dlg/dsselect.cxx")
+            return;
 
         TraverseDecl(compiler.getASTContext().getTranslationUnitDecl());
     }
@@ -72,6 +85,10 @@ bool UnnecessaryOverride::VisitCXXMethodDecl(const CXXMethodDecl* methodDecl)
     // entertaining template magic
     if (aFileName == SRCDIR "/sc/source/ui/vba/vbaformatcondition.cxx")
         return true;
+    // not sure what is going on here, but removing the override causes a crash
+     if (methodDecl->getQualifiedNameAsString() == "SwXTextDocument::queryAdapter")
+         return true;
+
 
     const CXXMethodDecl* overriddenMethodDecl = *methodDecl->begin_overridden_methods();
 
@@ -80,62 +97,78 @@ bool UnnecessaryOverride::VisitCXXMethodDecl(const CXXMethodDecl* methodDecl)
     {
         return true;
     }
+    if (methodDecl->getAccess() == AS_public && overriddenMethodDecl->getAccess() == AS_protected)
+        return true;
+
     //TODO: check for identical exception specifications
 
     const CompoundStmt* compoundStmt = dyn_cast<CompoundStmt>(methodDecl->getBody());
     if (!compoundStmt || compoundStmt->size() != 1)
         return true;
-    auto returnStmt = dyn_cast<ReturnStmt>(*compoundStmt->body_begin());
-    if (returnStmt == nullptr) {
-        return true;
-    }
-    auto returnExpr = returnStmt->getRetValue();
-    if (returnExpr == nullptr) {
-        return true;
+
+    const CXXMemberCallExpr* callExpr;
+    if (compat::getReturnType(*methodDecl).getCanonicalType()->isVoidType())
+    {
+        callExpr = dyn_cast<CXXMemberCallExpr>(*compoundStmt->body_begin());
     }
-    returnExpr = returnExpr->IgnoreImplicit();
-
-    // In something like
-    //
-    //  Reference< XResultSet > SAL_CALL OPreparedStatement::executeQuery(
-    //      const rtl::OUString& sql)
-    //      throw(SQLException, RuntimeException, std::exception)
-    //  {
-    //      return OCommonStatement::executeQuery( sql );
-    //  }
-    //
-    // look down through all the
-    //
-    //   ReturnStmt
-    //   `-ExprWithCleanups
-    //     `-CXXConstructExpr
-    //      `-MaterializeTemporaryExpr
-    //       `-ImplicitCastExpr
-    //        `-CXXBindTemporaryExpr
-    //         `-CXXMemberCallExpr
-    //
-    // where the fact that the overriding and overridden function have identical
-    // return types makes us confident that all we need to check here is whether
-    // there's an (arbitrary, one-argument) CXXConstructorExpr and
-    // CXXBindTemporaryExpr in between:
-    if (auto ctorExpr = dyn_cast<CXXConstructExpr>(returnExpr)) {
-        if (ctorExpr->getNumArgs() == 1) {
-            if (auto tempExpr = dyn_cast<CXXBindTemporaryExpr>(
-                    ctorExpr->getArg(0)->IgnoreImplicit()))
-            {
-                returnExpr = tempExpr->getSubExpr();
+    else
+    {
+        auto returnStmt = dyn_cast<ReturnStmt>(*compoundStmt->body_begin());
+        if (returnStmt == nullptr) {
+            return true;
+        }
+        auto returnExpr = returnStmt->getRetValue();
+        if (returnExpr == nullptr) {
+            return true;
+        }
+        returnExpr = returnExpr->IgnoreImplicit();
+
+        // In something like
+        //
+        //  Reference< XResultSet > SAL_CALL OPreparedStatement::executeQuery(
+        //      const rtl::OUString& sql)
+        //      throw(SQLException, RuntimeException, std::exception)
+        //  {
+        //      return OCommonStatement::executeQuery( sql );
+        //  }
+        //
+        // look down through all the
+        //
+        //   ReturnStmt
+        //   `-ExprWithCleanups
+        //     `-CXXConstructExpr
+        //      `-MaterializeTemporaryExpr
+        //       `-ImplicitCastExpr
+        //        `-CXXBindTemporaryExpr
+        //         `-CXXMemberCallExpr
+        //
+        // where the fact that the overriding and overridden function have identical
+        // return types makes us confident that all we need to check here is whether
+        // there's an (arbitrary, one-argument) CXXConstructorExpr and
+        // CXXBindTemporaryExpr in between:
+        if (auto ctorExpr = dyn_cast<CXXConstructExpr>(returnExpr)) {
+            if (ctorExpr->getNumArgs() == 1) {
+                auto tempExpr1 = ctorExpr->getArg(0)->IgnoreImplicit();
+                if (auto tempExpr2 = dyn_cast<CXXBindTemporaryExpr>(tempExpr1))
+                {
+                    returnExpr = tempExpr2->getSubExpr();
+                }
+                else if (auto tempExpr2 = dyn_cast<CXXMemberCallExpr>(tempExpr1))
+                {
+                    returnExpr = tempExpr2;
+                }
             }
         }
+
+        callExpr = dyn_cast<CXXMemberCallExpr>(returnExpr->IgnoreParenImpCasts());
     }
 
-    const CXXMemberCallExpr* callExpr = dyn_cast<CXXMemberCallExpr>(
-        returnExpr->IgnoreParenImpCasts());
     if (!callExpr || callExpr->getMethodDecl() != overriddenMethodDecl)
         return true;
-    const ImplicitCastExpr* expr1 = dyn_cast_or_null<ImplicitCastExpr>(callExpr->getImplicitObjectArgument());
+    const Expr* expr1 = callExpr->getImplicitObjectArgument()->IgnoreImpCasts();
     if (!expr1)
         return true;
-    const CXXThisExpr* expr2 = dyn_cast_or_null<CXXThisExpr>(expr1->getSubExpr());
+    const CXXThisExpr* expr2 = dyn_cast_or_null<CXXThisExpr>(expr1);
     if (!expr2)
         return true;
     for (unsigned i = 0; i<callExpr->getNumArgs(); ++i) {
@@ -146,9 +179,10 @@ bool UnnecessaryOverride::VisitCXXMethodDecl(const CXXMethodDecl* methodDecl)
     }
 
     report(
-        DiagnosticsEngine::Warning, "%0 virtual function just calls %1 parent",
-        methodDecl->getSourceRange().getBegin())
-        << methodDecl->getAccess() << overriddenMethodDecl->getAccess()
+            DiagnosticsEngine::Warning, "%0 virtual function just calls %1 parent",
+            methodDecl->getSourceRange().getBegin())
+        << methodDecl->getAccess()
+        << overriddenMethodDecl->getAccess()
         << methodDecl->getSourceRange();
     if (methodDecl->getCanonicalDecl()->getLocation() != methodDecl->getLocation()) {
         const CXXMethodDecl* pOther = methodDecl->getCanonicalDecl();