loplugin:unnecessaryoverride: suppress warnings when default args differ

...instead of blacklisting such cases.  Reuses the
checkIdenticalDefaultArguments code that was originally in
loplugin:overrideparam (and appears to work reasonably well for the default
arguments that actually happen in practice).

Change-Id: I9cf2db17101beb135b2039a9b7ed335bd2af2c08
Reviewed-on: https://gerrit.libreoffice.org/44594
Tested-by: Jenkins <ci@libreoffice.org>
Reviewed-by: Stephan Bergmann <sbergman@redhat.com>

5 files changed, 125 insertions, 50 deletions

diff --git a/compilerplugins/clang/overrideparam.cxx b/compilerplugins/clang/overrideparam.cxx
index e3b8a4e0fd88..820127e1343d 100644
--- a/compilerplugins/clang/overrideparam.cxx
+++ b/compilerplugins/clang/overrideparam.cxx
@@ -38,7 +38,6 @@ public:
 
 private:
     bool hasSameDefaultParams(const ParmVarDecl * parmVarDecl, const ParmVarDecl * superParmVarDecl);
-    bool evaluate(const Expr* expr, APSInt& x);
 };
 
 void OverrideParam::run()
@@ -148,39 +147,10 @@ bool OverrideParam::hasSameDefaultParams(const ParmVarDecl * parmVarDecl, const
     if (parmVarDecl->hasUninstantiatedDefaultArg() || superParmVarDecl->hasUninstantiatedDefaultArg()) {
         return true;
     }
-    const Expr* defaultArgExpr = parmVarDecl->getDefaultArg();
-    const Expr* superDefaultArgExpr = superParmVarDecl->getDefaultArg();
-
-    if (defaultArgExpr->isNullPointerConstant(compiler.getASTContext(), Expr::NPC_NeverValueDependent)
-        && superDefaultArgExpr->isNullPointerConstant(compiler.getASTContext(), Expr::NPC_NeverValueDependent))
-    {
-        return true;
-    }
-    APSInt x1, x2;
-    if (evaluate(defaultArgExpr, x1) && evaluate(superDefaultArgExpr, x2))
-    {
-        return x1 == x2;
-    }
-#if CLANG_VERSION >= 30900
-    APFloat f1(0.0f), f2(0.0f);
-    if (defaultArgExpr->EvaluateAsFloat(f1, compiler.getASTContext())
-        && superDefaultArgExpr->EvaluateAsFloat(f2, compiler.getASTContext()))
-    {
-        return f1.bitwiseIsEqual(f2);
-    }
-#endif
-    // catch params with defaults like "= OUString()"
-    if (isa<MaterializeTemporaryExpr>(defaultArgExpr)
-        && isa<MaterializeTemporaryExpr>(superDefaultArgExpr))
-    {
-        return true;
-    }
-    if (isa<CXXBindTemporaryExpr>(defaultArgExpr)
-        && isa<CXXBindTemporaryExpr>(superDefaultArgExpr))
-    {
-        return true;
-    }
-    return true;
+    return
+        checkIdenticalDefaultArguments(
+            parmVarDecl->getDefaultArg(), superParmVarDecl->getDefaultArg())
+        != IdenticalDefaultArgumentsResult::No;
         // for one, Clang 3.8 doesn't have EvaluateAsFloat; for another, since
         // <http://llvm.org/viewvc/llvm-project?view=revision&revision=291318>
         // "PR23135: Don't instantiate constexpr functions referenced in
@@ -196,19 +166,6 @@ bool OverrideParam::hasSameDefaultParams(const ParmVarDecl * parmVarDecl, const
         // that would probably have unwanted side-effects)
 }
 
-bool OverrideParam::evaluate(const Expr* expr, APSInt& x)
-{
-    if (expr->EvaluateAsInt(x, compiler.getASTContext()))
-    {
-        return true;
-    }
-    if (isa<CXXNullPtrLiteralExpr>(expr)) {
-        x = 0;
-        return true;
-    }
-    return false;
-}
-
 loplugin::Plugin::Registration< OverrideParam > X("overrideparam");
 
 }
diff --git a/compilerplugins/clang/plugin.cxx b/compilerplugins/clang/plugin.cxx
index f8292ef661f6..6697f94b3ee8 100644
--- a/compilerplugins/clang/plugin.cxx
+++ b/compilerplugins/clang/plugin.cxx
@@ -75,6 +75,19 @@ void Plugin::registerPlugin( Plugin* (*create)( const InstantiationData& ), cons
     PluginHandler::registerPlugin( create, optionName, isPPCallback, byDefault );
 }
 
+bool Plugin::evaluate(const Expr* expr, APSInt& x)
+{
+    if (expr->EvaluateAsInt(x, compiler.getASTContext()))
+    {
+        return true;
+    }
+    if (isa<CXXNullPtrLiteralExpr>(expr)) {
+        x = 0;
+        return true;
+    }
+    return false;
+}
+
 const Stmt* Plugin::getParentStmt( const Stmt* stmt )
 {
     auto parentsRange = compiler.getASTContext().getParents(*stmt);
@@ -203,6 +216,62 @@ bool Plugin::containsPreprocessingConditionalInclusion(SourceRange range)
     return false;
 }
 
+Plugin::IdenticalDefaultArgumentsResult Plugin::checkIdenticalDefaultArguments(
+    Expr const * argument1, Expr const * argument2)
+{
+    if ((argument1 == nullptr) != (argument2 == nullptr)) {
+        return IdenticalDefaultArgumentsResult::No;
+    }
+    if (argument1 == nullptr) {
+        return IdenticalDefaultArgumentsResult::Yes;
+    }
+    if (argument1->isNullPointerConstant(compiler.getASTContext(), Expr::NPC_NeverValueDependent)
+        && argument2->isNullPointerConstant(compiler.getASTContext(), Expr::NPC_NeverValueDependent))
+    {
+        return IdenticalDefaultArgumentsResult::Yes;
+    }
+    APSInt x1, x2;
+    if (evaluate(argument1, x1) && evaluate(argument2, x2))
+    {
+        return x1 == x2
+            ? IdenticalDefaultArgumentsResult::Yes
+            : IdenticalDefaultArgumentsResult::No;
+    }
+#if CLANG_VERSION >= 30900
+    APFloat f1(0.0f), f2(0.0f);
+    if (argument1->EvaluateAsFloat(f1, compiler.getASTContext())
+        && argument2->EvaluateAsFloat(f2, compiler.getASTContext()))
+    {
+        return f1.bitwiseIsEqual(f2)
+            ? IdenticalDefaultArgumentsResult::Yes
+            : IdenticalDefaultArgumentsResult::No;
+    }
+#endif
+    if (auto const lit1 = dyn_cast<clang::StringLiteral>(
+            argument1->IgnoreParenImpCasts()))
+    {
+        if (auto const lit2 = dyn_cast<clang::StringLiteral>(
+                argument2->IgnoreParenImpCasts()))
+        {
+            return lit1->getBytes() == lit2->getBytes()
+                ? IdenticalDefaultArgumentsResult::Yes
+                : IdenticalDefaultArgumentsResult::No;
+        }
+    }
+    // catch params with defaults like "= OUString()"
+    if (isa<MaterializeTemporaryExpr>(argument1)
+        && isa<MaterializeTemporaryExpr>(argument2))
+    {
+        return IdenticalDefaultArgumentsResult::Yes;
+    }
+    if (isa<CXXBindTemporaryExpr>(argument1)
+        && isa<CXXBindTemporaryExpr>(argument2))
+    {
+        return IdenticalDefaultArgumentsResult::Yes;
+    }
+    return IdenticalDefaultArgumentsResult::Maybe;
+}
+
 RewritePlugin::RewritePlugin( const InstantiationData& data )
     : Plugin( data )
     , rewriter( data.rewriter )
diff --git a/compilerplugins/clang/plugin.hxx b/compilerplugins/clang/plugin.hxx
index 8a8f6f60732e..02897031cd7b 100644
--- a/compilerplugins/clang/plugin.hxx
+++ b/compilerplugins/clang/plugin.hxx
@@ -82,9 +82,15 @@ protected:
 
     bool containsPreprocessingConditionalInclusion(SourceRange range);
 
+    enum class IdenticalDefaultArgumentsResult { No, Yes, Maybe };
+    IdenticalDefaultArgumentsResult checkIdenticalDefaultArguments(
+        Expr const * argument1, Expr const * argument2);
+
 private:
     static void registerPlugin( Plugin* (*create)( const InstantiationData& ), const char* optionName, bool isPPCallback, bool byDefault );
     template< typename T > static Plugin* createHelper( const InstantiationData& data );
+    bool evaluate(const Expr* expr, APSInt& x);
+
     enum { isRewriter = false };
     const char* name;
 };
diff --git a/compilerplugins/clang/test/unnecessaryoverride.cxx b/compilerplugins/clang/test/unnecessaryoverride.cxx
index 91e8a4003296..816feb9d6af4 100644
--- a/compilerplugins/clang/test/unnecessaryoverride.cxx
+++ b/compilerplugins/clang/test/unnecessaryoverride.cxx
@@ -7,6 +7,10 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/.
  */
 
+#include <sal/config.h>
+
+#include <config_clang.h>
+
 struct Base
 {
     virtual ~Base();
@@ -15,6 +19,7 @@ struct Base
     void cv() const volatile;
     void ref();
     static void staticFn();
+    void defaults(void* = nullptr, int = 0, double = 1, Base const& = {}, char const* = "foo");
 };
 
 struct SimpleDerived : Base
@@ -55,6 +60,34 @@ struct DerivedDifferent : Base
     void cv() { Base::cv(); } // no warning
     void ref() && { Base::ref(); } // no warning
     void staticFn() { Base::staticFn(); } // no warning
+    void defaults(void* x1, int x2, double x3, Base const& x4, char const* x5)
+    {
+        Base::defaults(x1, x2, x3, x4, x5); // no warning
+    }
+};
+
+struct DerivedSame : Base
+{
+#if CLANG_VERSION >= 30900 // cf. corresponding condition in Plugin::checkIdenticalDefaultArguments
+    void
+    defaults( // expected-error {{public function just calls public parent [loplugin:unnecessaryoverride]}}
+        void* x1 = 0, int x2 = (1 - 1), double x3 = 1.0, Base const& x4 = (Base()),
+        char const* x5 = "f"
+                         "oo")
+    {
+        Base::defaults(x1, x2, x3, x4, x5);
+    }
+#endif
+};
+
+struct DerivedSlightlyDifferent : Base
+{
+    void defaults( // no warning
+        void* x1 = nullptr, int x2 = 0, double x3 = 1, Base const& x4 = DerivedSlightlyDifferent(),
+        char const* x5 = "foo")
+    {
+        Base::defaults(x1, x2, x3, x4, x5);
+    }
 };
 
 /* 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 07f902158efd..13ed723385d8 100644
--- a/compilerplugins/clang/unnecessaryoverride.cxx
+++ b/compilerplugins/clang/unnecessaryoverride.cxx
@@ -258,15 +258,25 @@ bool UnnecessaryOverride::VisitCXXMethodDecl(const CXXMethodDecl* methodDecl)
             return true;
         }
     }
-    // some very creative method hiding going on here
-    if (loplugin::isSamePathname(aFileName, SRCDIR "/svx/source/dialog/checklbx.cxx"))
-        return true;
 
     const CXXMethodDecl* overriddenMethodDecl = findOverriddenOrSimilarMethodInSuperclasses(methodDecl);
     if (!overriddenMethodDecl) {
         return true;
     }
 
+    // Check for differences in default parameters:
+    unsigned const numParams = methodDecl->getNumParams();
+    assert(overriddenMethodDecl->getNumParams() == numParams);
+    for (unsigned i = 0; i != numParams; ++i) {
+        if (checkIdenticalDefaultArguments(
+                methodDecl->getParamDecl(i)->getDefaultArg(),
+                overriddenMethodDecl->getParamDecl(i)->getDefaultArg())
+            != IdenticalDefaultArgumentsResult::Yes)
+        {
+            return true;
+        }
+    }
+
     if (compat::getReturnType(*methodDecl).getCanonicalType()
         != compat::getReturnType(*overriddenMethodDecl).getCanonicalType())
     {