loplugin:redundantfcast improvements

check for calls to constructors, and extend the list of types we check
for unnecessary temporary creation

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

1 files changed, 81 insertions, 8 deletions

diff --git a/compilerplugins/clang/redundantfcast.cxx b/compilerplugins/clang/redundantfcast.cxx
index 3dc99d633626..43049700ee35 100644
--- a/compilerplugins/clang/redundantfcast.cxx
+++ b/compilerplugins/clang/redundantfcast.cxx
@@ -10,6 +10,8 @@
 #include "check.hxx"
 #include "compat.hxx"
 #include "plugin.hxx"
+#include <iostream>
+#include <fstream>
 
 namespace
 {
@@ -118,35 +120,106 @@ public:
         return true;
     }
 
+    /* Check for the creation of unnecessary temporaries when calling a constructor that takes a param by const & */
+    bool VisitCXXConstructExpr(CXXConstructExpr const* callExpr)
+    {
+        if (ignoreLocation(callExpr))
+            return true;
+        const CXXConstructorDecl* functionDecl = callExpr->getConstructor();
+
+        unsigned len = std::min(callExpr->getNumArgs(), functionDecl->getNumParams());
+        for (unsigned i = 0; i < len; ++i)
+        {
+            // check if param is const&
+            auto param = functionDecl->getParamDecl(i);
+            auto lvalueType = param->getType()->getAs<LValueReferenceType>();
+            if (!lvalueType)
+                continue;
+            if (!lvalueType->getPointeeType().isConstQualified())
+                continue;
+            auto paramClassOrStructType = lvalueType->getPointeeType()->getAs<RecordType>();
+            if (!paramClassOrStructType)
+                continue;
+            // check for temporary and functional cast in argument expression
+            auto arg = callExpr->getArg(i)->IgnoreImplicit();
+            auto functionalCast = dyn_cast<CXXFunctionalCastExpr>(arg);
+            if (!functionalCast)
+                continue;
+            auto const t1 = functionalCast->getTypeAsWritten();
+            auto const t2 = compat::getSubExprAsWritten(functionalCast)->getType();
+            if (t1.getCanonicalType().getTypePtr() != t2.getCanonicalType().getTypePtr())
+                continue;
+            // Check that the underlying expression is of the same class/struct type as the param i.e. that we are not instantiating
+            // something useful
+            if (t1.getCanonicalType().getTypePtr() != paramClassOrStructType)
+                continue;
+
+            report(DiagnosticsEngine::Warning, "redundant functional cast from %0 to %1",
+                   arg->getExprLoc())
+                << t2 << t1 << arg->getSourceRange();
+            report(DiagnosticsEngine::Note, "in call to method here", param->getLocation())
+                << param->getSourceRange();
+        }
+        return true;
+    }
+
     bool VisitCXXFunctionalCastExpr(CXXFunctionalCastExpr const* expr)
     {
         if (ignoreLocation(expr))
             return true;
+        // specifying the name for an init-list is necessary sometimes
+        if (isa<InitListExpr>(expr->getSubExpr()->IgnoreImplicit()))
+            return true;
+        if (isa<CXXStdInitializerListExpr>(expr->getSubExpr()->IgnoreImplicit()))
+            return true;
         auto const t1 = expr->getTypeAsWritten();
         auto const t2 = compat::getSubExprAsWritten(expr)->getType();
         if (t1.getCanonicalType().getTypePtr() != t2.getCanonicalType().getTypePtr())
         {
             return true;
         }
-        auto tc = loplugin::TypeCheck(t1);
-        if (!(tc.Class("OUString").Namespace("rtl").GlobalNamespace()
-              || tc.Class("Color").GlobalNamespace() || tc.Class("unique_ptr").StdNamespace()))
+        // (a) we do a lot of int/sal_Int32 kind of casts which might be platform necessary?
+        // (b) we do bool/bool casts in unit tests to avoid one of the other plugins
+        // so just ignore this kind of thing for now
+        if (const auto* BT = dyn_cast<BuiltinType>(t1->getUnqualifiedDesugaredType()))
         {
-            return true;
+            auto k = BT->getKind();
+            if (k == BuiltinType::Double || k == BuiltinType::Float
+                || (k >= BuiltinType::Bool && k <= BuiltinType::Int128))
+                return true;
+        }
+        if (const auto* BT = dyn_cast<BuiltinType>(t2->getUnqualifiedDesugaredType()))
+        {
+            auto k = BT->getKind();
+            if (k == BuiltinType::Double || k == BuiltinType::Float
+                || (k >= BuiltinType::Bool && k <= BuiltinType::Int128))
+                return true;
         }
+        auto tc = loplugin::TypeCheck(t1);
+        if (tc.Typedef("sal_Int32").GlobalNamespace())
+            return true;
+
         report(DiagnosticsEngine::Warning, "redundant functional cast from %0 to %1",
                expr->getExprLoc())
             << t2 << t1 << expr->getSourceRange();
+        //getParentStmt(expr)->dump();
         return true;
     }
 
 private:
     void run() override
     {
-        if (compiler.getLangOpts().CPlusPlus)
-        {
-            TraverseDecl(compiler.getASTContext().getTranslationUnitDecl());
-        }
+        if (!compiler.getLangOpts().CPlusPlus)
+            return;
+        std::string fn = handler.getMainFileName();
+        loplugin::normalizeDotDotInFilePath(fn);
+        // necessary on some other platforms
+        if (fn == SRCDIR "/sal/osl/unx/socket.cxx")
+            return;
+        // compile-time check of constant
+        if (fn == SRCDIR "/bridges/source/jni_uno/jni_bridge.cxx")
+            return;
+        TraverseDecl(compiler.getASTContext().getTranslationUnitDecl());
     }
     FunctionDecl const* m_CurrentFunctionDecl;
 };