loplugin:redundantfcast ignore necessary temporaries

when passing data to a method that is of type
   Foo&&

Change-Id: I0e6bcfb42d6ebcbc7cb19e510ab2010a2cc2bb7e
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/121843
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>

3 files changed, 53 insertions, 14 deletions

diff --git a/compilerplugins/clang/compat.hxx b/compilerplugins/clang/compat.hxx
index d04538e164b7..54f0d701a023 100644
--- a/compilerplugins/clang/compat.hxx
+++ b/compilerplugins/clang/compat.hxx
@@ -172,6 +172,11 @@ inline clang::Expr const * IgnoreImplicit(clang::Expr const * expr) {
 #endif
 }
 
+/// Utility method
+inline clang::Expr const * IgnoreParenImplicit(clang::Expr const * expr) {
+    return compat::IgnoreImplicit(compat::IgnoreImplicit(expr)->IgnoreParens());
+}
+
 inline bool CPlusPlus17(clang::LangOptions const & opts) {
 #if CLANG_VERSION >= 60000
     return opts.CPlusPlus17;
diff --git a/compilerplugins/clang/redundantfcast.cxx b/compilerplugins/clang/redundantfcast.cxx
index 758aa6b611da..4952d37caf79 100644
--- a/compilerplugins/clang/redundantfcast.cxx
+++ b/compilerplugins/clang/redundantfcast.cxx
@@ -137,16 +137,21 @@ public:
         {
             // 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>();
+            auto rvalueType = param->getType()->getAs<RValueReferenceType>();
+            if (!rvalueType)
+            {
+                auto lvalueType = param->getType()->getAs<LValueReferenceType>();
+                if (!lvalueType)
+                    continue;
+                if (!lvalueType->getPointeeType().isConstQualified())
+                    continue;
+            }
+            auto valueType = param->getType()->getAs<ReferenceType>();
+            auto paramClassOrStructType = valueType->getPointeeType()->getAs<RecordType>();
             if (!paramClassOrStructType)
                 continue;
             // check for temporary and functional cast in argument expression
-            auto arg = callExpr->getArg(i)->IgnoreImplicit();
+            auto arg = compat::IgnoreParenImplicit(callExpr->getArg(i));
             auto functionalCast = dyn_cast<CXXFunctionalCastExpr>(arg);
             if (!functionalCast)
                 continue;
@@ -158,10 +163,19 @@ public:
             // something useful
             if (t1.getCanonicalType().getTypePtr() != paramClassOrStructType)
                 continue;
+            if (rvalueType)
+            {
+                // constructing a temporary to pass to a && argument is fine. But we will see that in the VisitFunctionalCast
+                // method below and generate a warning. And we don't have enough context there to determine that we're
+                // doing the wrong thing. So add the expression to the m_Seen list here to prevent that warning.
+                m_Seen.insert(functionalCast->getExprLoc());
+                continue;
+            }
 
             if (m_Seen.insert(arg->getExprLoc()).second)
             {
-                report(DiagnosticsEngine::Warning, "redundant functional cast from %0 to %1",
+                report(DiagnosticsEngine::Warning,
+                       "redundant functional cast from %0 to %1 in construct expression",
                        arg->getExprLoc())
                     << t2 << t1 << arg->getSourceRange();
                 report(DiagnosticsEngine::Note, "in call to method here", param->getLocation())
@@ -234,10 +248,12 @@ public:
         {
             return false;
         }
-        auto cxxConstruct = dyn_cast<CXXConstructExpr>(compat::IgnoreImplicit(expr->getSubExpr()));
+        auto cxxConstruct
+            = dyn_cast<CXXConstructExpr>(compat::IgnoreParenImplicit(expr->getSubExpr()));
         if (!cxxConstruct)
             return false;
-        auto const lambda = dyn_cast<LambdaExpr>(cxxConstruct->getArg(0)->IgnoreImplicit());
+        auto const lambda
+            = dyn_cast<LambdaExpr>(compat::IgnoreParenImplicit(cxxConstruct->getArg(0)));
         if (!lambda)
             return false;
         if (deduced)
@@ -261,9 +277,9 @@ public:
         if (ignoreLocation(expr))
             return true;
         // specifying the name for an init-list is necessary sometimes
-        if (isa<InitListExpr>(expr->getSubExpr()->IgnoreImplicit()))
+        if (isa<InitListExpr>(compat::IgnoreParenImplicit(expr->getSubExpr())))
             return true;
-        if (isa<CXXStdInitializerListExpr>(expr->getSubExpr()->IgnoreImplicit()))
+        if (isa<CXXStdInitializerListExpr>(compat::IgnoreParenImplicit(expr->getSubExpr())))
             return true;
         auto const t1 = expr->getTypeAsWritten();
         auto const t2 = compat::getSubExprAsWritten(expr)->getType();
@@ -297,7 +313,6 @@ public:
             report(DiagnosticsEngine::Warning, "redundant functional cast from %0 to %1",
                    expr->getExprLoc())
                 << t2 << t1 << expr->getSourceRange();
-        //getParentStmt(expr)->dump();
         return true;
     }
 
diff --git a/compilerplugins/clang/test/redundantfcast.cxx b/compilerplugins/clang/test/redundantfcast.cxx
index d9aad3619075..20c939cb2b42 100644
--- a/compilerplugins/clang/test/redundantfcast.cxx
+++ b/compilerplugins/clang/test/redundantfcast.cxx
@@ -74,7 +74,7 @@ int main()
 
     const tools::Polygon aPolygon;
     ImplWritePolyPolygonRecord(tools::PolyPolygon(tools::Polygon(
-        aPolygon))); // expected-error@-1 {{redundant functional cast from 'const tools::Polygon' to 'tools::Polygon' [loplugin:redundantfcast]}}
+        aPolygon))); // expected-error@-1 {{redundant functional cast from 'const tools::Polygon' to 'tools::Polygon' in construct expression [loplugin:redundantfcast]}}
 }
 
 class Class1
@@ -202,4 +202,23 @@ void g(std::initializer_list<int> il)
 }
 }
 
+namespace test8
+{
+class Primitive2DContainer
+{
+};
+struct GroupPrimitive
+{
+    GroupPrimitive(Primitive2DContainer&&);
+};
+
+const Primitive2DContainer& getChildren();
+
+void foo()
+{
+    // no warning expected, we have to create a temporary for this constructor
+    GroupPrimitive aGroup((Primitive2DContainer(getChildren())));
+    (void)aGroup;
+}
+}
 /* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s cinkeys+=0=break: */