From cab6e6836973a9ddfc5ed9df757e07138328c1c3 Mon Sep 17 00:00:00 2001
From: Stephan Bergmann <sbergman@redhat.com>
Date: Tue, 14 Nov 2017 19:17:07 +0100
Subject: Make checkIdenticalDefaultArguments more precise

...when creating objects involves copy/move constructors

Change-Id: I0c7ccb85b7dcb584502a48817d7d2abfde25aaf2
Reviewed-on: https://gerrit.libreoffice.org/44733
Tested-by: Jenkins <ci@libreoffice.org>
Reviewed-by: Stephan Bergmann <sbergman@redhat.com>
---
 compilerplugins/clang/plugin.cxx                   | 32 ++++++++++++++--------
 compilerplugins/clang/test/unnecessaryoverride.cxx |  7 +++++
 compilerplugins/clang/unnecessaryoverride.cxx      | 15 +++++++---
 3 files changed, 38 insertions(+), 16 deletions(-)

(limited to 'compilerplugins')

diff --git a/compilerplugins/clang/plugin.cxx b/compilerplugins/clang/plugin.cxx
index 717d88b23091..80fff7651ae0 100644
--- a/compilerplugins/clang/plugin.cxx
+++ b/compilerplugins/clang/plugin.cxx
@@ -30,7 +30,7 @@ namespace {
 
 Expr const * skipImplicit(Expr const * expr) {
     if (auto const e = dyn_cast<MaterializeTemporaryExpr>(expr)) {
-        expr = e->GetTemporaryExpr();
+        expr = e->GetTemporaryExpr()->IgnoreImpCasts();
     }
     if (auto const e = dyn_cast<CXXBindTemporaryExpr>(expr)) {
         expr = e->getSubExpr();
@@ -271,18 +271,26 @@ Plugin::IdenticalDefaultArgumentsResult Plugin::checkIdenticalDefaultArguments(
         }
     }
     // catch params with defaults like "= OUString()"
-    if (auto const e1 = dyn_cast<CXXConstructExpr>(skipImplicit(desugared1))) {
-        if (auto const e2 = dyn_cast<CXXConstructExpr>(
-                skipImplicit(desugared2)))
+    for (Expr const * e1 = desugared1, * e2 = desugared2;;) {
+        auto const ce1 = dyn_cast<CXXConstructExpr>(skipImplicit(e1));
+        auto const ce2 = dyn_cast<CXXConstructExpr>(skipImplicit(e2));
+        if (ce1 == nullptr || ce2 == nullptr) {
+            break;
+        }
+        if (ce1->getConstructor()->getCanonicalDecl() != ce2->getConstructor()->getCanonicalDecl())
         {
-            if ((e1->getConstructor()->getCanonicalDecl()
-                 != e2->getConstructor()->getCanonicalDecl()))
-            {
-                return IdenticalDefaultArgumentsResult::No;
-            }
-            if (e1->getNumArgs() == 0 && e2->getNumArgs() == 0) {
-                return IdenticalDefaultArgumentsResult::Yes;
-            }
+            return IdenticalDefaultArgumentsResult::No;
+        }
+        if (ce1->isElidable() && ce2->isElidable() && ce1->getNumArgs() == 1
+            && ce2->getNumArgs() == 1)
+        {
+            assert(ce1->getConstructor()->isCopyOrMoveConstructor());
+            e1 = ce1->getArg(0)->IgnoreImpCasts();
+            e2 = ce2->getArg(0)->IgnoreImpCasts();
+            continue;
+        }
+        if (ce1->getNumArgs() == 0 && ce2->getNumArgs() == 0) {
+            return IdenticalDefaultArgumentsResult::Yes;
         }
     }
     return IdenticalDefaultArgumentsResult::Maybe;
diff --git a/compilerplugins/clang/test/unnecessaryoverride.cxx b/compilerplugins/clang/test/unnecessaryoverride.cxx
index 7abcf971986f..1f5c1f9fb4a7 100644
--- a/compilerplugins/clang/test/unnecessaryoverride.cxx
+++ b/compilerplugins/clang/test/unnecessaryoverride.cxx
@@ -94,6 +94,7 @@ struct Base2
 {
     void default1(Base const& = SimpleDerived());
     void default2(Base const& = SimpleDerived());
+    void default3(Base = Base());
 };
 
 struct Derived2 : Base2
@@ -105,6 +106,12 @@ struct Derived2 : Base2
     {
         Base2::default2(x);
     }
+    void
+    default3( // expected-error {{public function just calls public parent [loplugin:unnecessaryoverride]}}
+        Base x = Base())
+    {
+        (Base2::default3(x));
+    }
 };
 
 /* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s cinkeys+=0=break: */
diff --git a/compilerplugins/clang/unnecessaryoverride.cxx b/compilerplugins/clang/unnecessaryoverride.cxx
index 13ed723385d8..336c7712a95f 100644
--- a/compilerplugins/clang/unnecessaryoverride.cxx
+++ b/compilerplugins/clang/unnecessaryoverride.cxx
@@ -289,10 +289,12 @@ bool UnnecessaryOverride::VisitCXXMethodDecl(const CXXMethodDecl* methodDecl)
     if (!compoundStmt || compoundStmt->size() != 1)
         return true;
 
-    const CXXMemberCallExpr* callExpr;
+    const CXXMemberCallExpr* callExpr = nullptr;
     if (compat::getReturnType(*methodDecl).getCanonicalType()->isVoidType())
     {
-        callExpr = dyn_cast<CXXMemberCallExpr>(*compoundStmt->body_begin());
+        if (auto const e = dyn_cast<Expr>(*compoundStmt->body_begin())) {
+            callExpr = dyn_cast<CXXMemberCallExpr>(e->IgnoreImplicit()->IgnoreParens());
+        }
     }
     else
     {
@@ -355,8 +357,13 @@ bool UnnecessaryOverride::VisitCXXMethodDecl(const CXXMethodDecl* methodDecl)
     if (!expr2)
         return true;
     for (unsigned i = 0; i<callExpr->getNumArgs(); ++i) {
-        // ignore ImplicitCastExpr
-        const DeclRefExpr * declRefExpr = dyn_cast<DeclRefExpr>(callExpr->getArg(i)->IgnoreImplicit());
+        auto e = callExpr->getArg(i)->IgnoreImplicit();
+        if (auto const e1 = dyn_cast<CXXConstructExpr>(e)) {
+            if (e1->getConstructor()->isCopyOrMoveConstructor() && e1->getNumArgs() == 1) {
+                e = e1->getArg(0)->IgnoreImpCasts();
+            }
+        }
+        const DeclRefExpr * declRefExpr = dyn_cast<DeclRefExpr>(e);
         if (!declRefExpr || declRefExpr->getDecl() != methodDecl->getParamDecl(i))
             return true;
     }
-- 
cgit