teach redundantcast plugin about functional casts

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

3 files changed, 66 insertions, 4 deletions

diff --git a/compilerplugins/clang/redundantcast.cxx b/compilerplugins/clang/redundantcast.cxx
index 3c38dd3ef495..148cdbead9be 100644
--- a/compilerplugins/clang/redundantcast.cxx
+++ b/compilerplugins/clang/redundantcast.cxx
@@ -87,6 +87,8 @@ public:
 
     bool VisitCXXConstCastExpr(CXXConstCastExpr const * expr);
 
+    bool VisitCXXFunctionalCastExpr(CXXFunctionalCastExpr const * expr);
+
     bool VisitCallExpr(CallExpr const * expr);
 
     bool VisitCXXDeleteExpr(CXXDeleteExpr const * expr);
@@ -434,6 +436,66 @@ bool RedundantCast::VisitCXXConstCastExpr(CXXConstCastExpr const * expr) {
     return true;
 }
 
+bool RedundantCast::VisitCXXFunctionalCastExpr(CXXFunctionalCastExpr const * expr) {
+    if (ignoreLocation(expr)) {
+        return true;
+    }
+    // A bit of a rabbit hole here,  these expressions look like
+    //   CPPUNIT_ASSERT( bool(aVec.find(p1.get()) == aVec.end()) )
+    // If I remove the bool, then another plugin wants me to change it to CPPUNIT_ASSERT_EQUAL,
+    // but that fails because CppUnit can't do "std::ostream << iterator".
+    StringRef aFileName = compiler.getSourceManager().getFilename(compiler.getSourceManager().getSpellingLoc(expr->getLocStart()));
+    if (loplugin::hasPathnamePrefix(aFileName, SRCDIR "/o3tl/qa/"))
+        return true;
+    if (loplugin::hasPathnamePrefix(aFileName, SRCDIR "/sfx2/qa/"))
+        return true;
+    if (loplugin::hasPathnamePrefix(aFileName, SRCDIR "/postprocess/qa/"))
+        return true;
+    if (loplugin::hasPathnamePrefix(aFileName, SRCDIR "/sc/qa/"))
+        return true;
+    if (loplugin::hasPathnamePrefix(aFileName, SRCDIR "/cppu/qa/"))
+        return true;
+    if (loplugin::hasPathnamePrefix(aFileName, SRCDIR "/vcl/qa/"))
+        return true;
+    if (loplugin::hasPathnamePrefix(aFileName, SRCDIR "/cppuhelper/qa/"))
+        return true;
+    if (loplugin::hasPathnamePrefix(aFileName, SRCDIR "/comphelper/qa/"))
+        return true;
+    if (loplugin::hasPathnamePrefix(aFileName, SRCDIR "/connectivity/qa/"))
+        return true;
+    if (loplugin::hasPathnamePrefix(aFileName, SRCDIR "/sal/qa/"))
+        return true;
+    if (loplugin::hasPathnamePrefix(aFileName, SRCDIR "/salhelper/qa/"))
+        return true;
+    if (loplugin::hasPathnamePrefix(aFileName, SRCDIR "/sw/qa/"))
+        return true;
+    if (loplugin::hasPathnamePrefix(aFileName, SRCDIR "/svl/qa/"))
+        return true;
+    // the array-of-struct initialiser here makes clang unhappy if I remove all of the "SchemeInfo" names
+    if (loplugin::hasPathnamePrefix(aFileName, SRCDIR "/tools/source/fsys/urlobj.cxx"))
+        return true;
+    // 2 structs with compiled-generated constructors where I cannot remove the cast even though the cast is a NoOp
+    if (loplugin::hasPathnamePrefix(aFileName, SRCDIR "/tools/source/inet/inetmime.cxx"))
+        return true;
+    // some explicit use of std::initializer_list
+    if (loplugin::hasPathnamePrefix(aFileName, SRCDIR "/svx/source/sidebar/area/AreaPropertyPanel.cxx"))
+        return true;
+    if (loplugin::hasPathnamePrefix(aFileName, SRCDIR "/svx/source/tbxctrls/fillctrl.cxx"))
+        return true;
+
+    auto const t1 = expr->getTypeAsWritten();
+    auto const t2 = compat::getSubExprAsWritten(expr)->getType();
+    if (t1 != t2)
+        return true;
+    if (!isOkToRemoveArithmeticCast(t1, t2, expr->getSubExpr()))
+        return true;
+    report(
+        DiagnosticsEngine::Warning,
+        "redundant functional cast from/to %0", expr->getExprLoc())
+        << t1 << expr->getSourceRange();
+    return true;
+}
+
 bool RedundantCast::VisitCallExpr(CallExpr const * expr) {
     if (ignoreLocation(expr)) {
         return true;
diff --git a/compilerplugins/clang/test/cppunitassertequals.cxx b/compilerplugins/clang/test/cppunitassertequals.cxx
index 239de58853b3..f0b03fa7ffb4 100644
--- a/compilerplugins/clang/test/cppunitassertequals.cxx
+++ b/compilerplugins/clang/test/cppunitassertequals.cxx
@@ -48,9 +48,9 @@ void test(bool b1, bool b2, OUString const & s1, OUString const & s2, T t) {
     CPPUNIT_ASSERT(t.operator ==(t));
 
     // There might even be good reasons(?) not to warn inside explicit casts:
-    CPPUNIT_ASSERT(bool(b1 && b2));
-    CPPUNIT_ASSERT(bool(b1 == b2));
-    CPPUNIT_ASSERT(bool(s1 == s2));
+    CPPUNIT_ASSERT(bool(b1 && b2)); // expected-error {{redundant functional cast from/to 'bool' [loplugin:redundantcast]}}
+    CPPUNIT_ASSERT(bool(b1 == b2)); // expected-error {{redundant functional cast from/to 'bool' [loplugin:redundantcast]}}
+    CPPUNIT_ASSERT(bool(s1 == s2)); // expected-error {{redundant functional cast from/to 'bool' [loplugin:redundantcast]}}
 }
 
 /* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s cinkeys+=0=break: */
diff --git a/compilerplugins/clang/test/stringcopy.cxx b/compilerplugins/clang/test/stringcopy.cxx
index c801b7096f74..f571f914b20e 100644
--- a/compilerplugins/clang/test/stringcopy.cxx
+++ b/compilerplugins/clang/test/stringcopy.cxx
@@ -13,7 +13,7 @@
 
 int main() {
     OUString s;
-    (void) OUString(s); // expected-error {{redundant copy construction from 'rtl::OUString' to 'rtl::OUString' [loplugin:stringcopy]}}
+    (void) OUString(s); // expected-error {{redundant copy construction from 'rtl::OUString' to 'rtl::OUString' [loplugin:stringcopy]}} expected-error {{redundant functional cast from/to 'rtl::OUString' [loplugin:redundantcast]}}
     using T1 = OUString;
     (void) T1(s); // expected-error {{redundant copy construction from 'rtl::OUString' to 'T1' (aka 'rtl::OUString') [loplugin:stringcopy]}}
     using T2 = OUString const;