Check isOkToRemoveArithmeticCast in loplugin:redundantfcast too

...to avoid warnings like

> C:/lo64/core/svx/source/table/accessiblecell.cxx(400,12):  error: redundant functional cast from 'long' to 'sal_Int32' (aka 'long') [loplugin:redundantfcast]
>     return sal_Int32(0x0ffffffL);
>            ^~~~~~~~~~~~~~~~~~~~~

with clang-cl

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

4 files changed, 65 insertions, 48 deletions

diff --git a/compilerplugins/clang/check.cxx b/compilerplugins/clang/check.cxx
index 2a7b0a978348..2f1ca066452f 100644
--- a/compilerplugins/clang/check.cxx
+++ b/compilerplugins/clang/check.cxx
@@ -228,6 +228,57 @@ bool isExtraWarnUnusedType(clang::QualType type) {
     return isDerivedFromSomethingInteresting(rec);
 }
 
+namespace {
+
+bool isArithmeticOp(clang::Expr const * expr) {
+    expr = expr->IgnoreParenImpCasts();
+    if (auto const e = llvm::dyn_cast<clang::BinaryOperator>(expr)) {
+        switch (e->getOpcode()) {
+        case clang::BO_Mul:
+        case clang::BO_Div:
+        case clang::BO_Rem:
+        case clang::BO_Add:
+        case clang::BO_Sub:
+        case clang::BO_Shl:
+        case clang::BO_Shr:
+        case clang::BO_And:
+        case clang::BO_Xor:
+        case clang::BO_Or:
+            return true;
+        case clang::BO_Comma:
+            return isArithmeticOp(e->getRHS());
+        default:
+            return false;
+        }
+    }
+    return llvm::isa<clang::UnaryOperator>(expr)
+        || llvm::isa<clang::AbstractConditionalOperator>(expr);
+}
+
+}
+
+bool isOkToRemoveArithmeticCast(
+    clang::ASTContext & context, clang::QualType t1, clang::QualType t2, const clang::Expr* subExpr)
+{
+    // Don't warn if the types are arithmetic (in the C++ meaning), and: either
+    // at least one is a typedef (and if both are typedefs,they're different),
+    // or the sub-expression involves some operation that is likely to change
+    // types through promotion, or the sub-expression is an integer literal (so
+    // its type generally depends on its value and suffix if any---even with a
+    // suffix like L it could still be either long or long long):
+    if ((t1->isIntegralType(context)
+         || t1->isRealFloatingType())
+        && ((t1 != t2
+             && (loplugin::TypeCheck(t1).Typedef()
+                 || loplugin::TypeCheck(t2).Typedef()))
+            || isArithmeticOp(subExpr)
+            || llvm::isa<clang::IntegerLiteral>(subExpr->IgnoreParenImpCasts())))
+    {
+        return false;
+    }
+    return true;
+}
+
 }
 
 /* vim:set shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/compilerplugins/clang/check.hxx b/compilerplugins/clang/check.hxx
index af6e8263df39..407258393eb8 100644
--- a/compilerplugins/clang/check.hxx
+++ b/compilerplugins/clang/check.hxx
@@ -283,6 +283,10 @@ template<std::size_t N> ContextCheck ContextCheck::Struct(char const (& id)[N])
 
 bool isExtraWarnUnusedType(clang::QualType type);
 
+bool isOkToRemoveArithmeticCast(
+    clang::ASTContext & context, clang::QualType t1, clang::QualType t2,
+    const clang::Expr* subExpr);
+
 }
 
 #endif
diff --git a/compilerplugins/clang/redundantcast.cxx b/compilerplugins/clang/redundantcast.cxx
index c4d50424b1a0..0faadc5a541a 100644
--- a/compilerplugins/clang/redundantcast.cxx
+++ b/compilerplugins/clang/redundantcast.cxx
@@ -45,30 +45,6 @@ bool isRedundantConstCast(CXXConstCastExpr const * expr) {
             || sub->getValueKind() == VK_XValue);
 }
 
-bool isArithmeticOp(Expr const * expr) {
-    expr = expr->IgnoreParenImpCasts();
-    if (auto const e = dyn_cast<BinaryOperator>(expr)) {
-        switch (e->getOpcode()) {
-        case BO_Mul:
-        case BO_Div:
-        case BO_Rem:
-        case BO_Add:
-        case BO_Sub:
-        case BO_Shl:
-        case BO_Shr:
-        case BO_And:
-        case BO_Xor:
-        case BO_Or:
-            return true;
-        case BO_Comma:
-            return isArithmeticOp(e->getRHS());
-        default:
-            return false;
-        }
-    }
-    return isa<UnaryOperator>(expr) || isa<AbstractConditionalOperator>(expr);
-}
-
 bool canConstCastFromTo(Expr const * from, Expr const * to) {
     auto const k1 = from->getValueKind();
     auto const k2 = to->getValueKind();
@@ -150,7 +126,6 @@ public:
 
 private:
     bool visitBinOp(BinaryOperator const * expr);
-    bool isOkToRemoveArithmeticCast(QualType t1, QualType t2, const Expr* subExpr);
     bool isOverloadedFunction(FunctionDecl const * decl);
 };
 
@@ -297,7 +272,8 @@ bool RedundantCast::VisitCStyleCastExpr(CStyleCastExpr const * expr) {
     if (!t1->isBuiltinType() && !loplugin::TypeCheck(t1).Enum() && !loplugin::TypeCheck(t1).Typedef()) {
         return true;
     }
-    if (!isOkToRemoveArithmeticCast(t1, t2, expr->getSubExpr())) {
+    if (!loplugin::isOkToRemoveArithmeticCast(compiler.getASTContext(), t1, t2, expr->getSubExpr()))
+    {
         return true;
     }
     // Ignore FD_ISSET expanding to "...(SOCKET)(fd)..." in some Microsoft
@@ -329,26 +305,6 @@ bool RedundantCast::VisitCStyleCastExpr(CStyleCastExpr const * expr) {
     return true;
 }
 
-bool RedundantCast::isOkToRemoveArithmeticCast(QualType t1, QualType t2, const Expr* subExpr) {
-    // Don't warn if the types are arithmetic (in the C++ meaning), and: either
-    // at least one is a typedef (and if both are typedefs,they're different),
-    // or the sub-expression involves some operation that is likely to change
-    // types through promotion, or the sub-expression is an integer literal (so
-    // its type generally depends on its value and suffix if any---even with a
-    // suffix like L it could still be either long or long long):
-    if ((t1->isIntegralType(compiler.getASTContext())
-         || t1->isRealFloatingType())
-        && ((t1 != t2
-             && (loplugin::TypeCheck(t1).Typedef()
-                 || loplugin::TypeCheck(t2).Typedef()))
-            || isArithmeticOp(subExpr)
-            || isa<IntegerLiteral>(subExpr->IgnoreParenImpCasts())))
-    {
-        return false;
-    }
-    return true;
-}
-
 bool RedundantCast::VisitCXXStaticCastExpr(CXXStaticCastExpr const * expr) {
     if (ignoreLocation(expr)) {
         return true;
@@ -396,7 +352,8 @@ bool RedundantCast::VisitCXXStaticCastExpr(CXXStaticCastExpr const * expr) {
             << expr->getSourceRange();
         return true;
     }
-    if (!isOkToRemoveArithmeticCast(t1, t2, expr->getSubExpr())) {
+    if (!loplugin::isOkToRemoveArithmeticCast(compiler.getASTContext(), t1, t2, expr->getSubExpr()))
+    {
         return true;
     }
     // Don't warn if the types are 'void *' and at least one involves a typedef
@@ -698,7 +655,7 @@ bool RedundantCast::VisitCXXFunctionalCastExpr(CXXFunctionalCastExpr const * exp
     auto const t2 = sub->getType();
     if (t1.getCanonicalType() != t2.getCanonicalType())
         return true;
-    if (!isOkToRemoveArithmeticCast(t1, t2, expr->getSubExpr()))
+    if (!loplugin::isOkToRemoveArithmeticCast(compiler.getASTContext(), t1, t2, expr->getSubExpr()))
         return true;
     report(
         DiagnosticsEngine::Warning,
diff --git a/compilerplugins/clang/redundantfcast.cxx b/compilerplugins/clang/redundantfcast.cxx
index 89e5618e19da..8a1e913c18d5 100644
--- a/compilerplugins/clang/redundantfcast.cxx
+++ b/compilerplugins/clang/redundantfcast.cxx
@@ -54,6 +54,11 @@ public:
         auto const t2 = compat::getSubExprAsWritten(cxxFunctionalCastExpr)->getType();
         if (t1.getCanonicalType().getTypePtr() != t2.getCanonicalType().getTypePtr())
             return true;
+        if (!loplugin::isOkToRemoveArithmeticCast(compiler.getASTContext(), t1, t2,
+                                                  cxxFunctionalCastExpr->getSubExpr()))
+        {
+            return true;
+        }
         report(DiagnosticsEngine::Warning, "redundant functional cast from %0 to %1",
                cxxFunctionalCastExpr->getExprLoc())
             << t2 << t1 << cxxFunctionalCastExpr->getSourceRange();