basic/source/sbx/sbxvalue.cxx | 103 +++++----- chart2/source/view/main/PropertyMapper.cxx | 2 compilerplugins/clang/test/unnecessaryparen.cxx | 6 compilerplugins/clang/test/vclwidgets.cxx | 2 compilerplugins/clang/unnecessaryparen.cxx | 6 cppu/qa/test_unotype.cxx | 2 cui/source/dialogs/about.cxx | 4 sc/source/ui/StatisticsDialogs/ExponentialSmoothingDialog.cxx | 2 sd/source/ui/remotecontrol/BluetoothServer.cxx | 5 solenv/gbuild/platform/com_GCC_defs.mk | 2 sw/source/core/doc/tblrwcl.cxx | 2 sw/source/core/undo/undobj1.cxx | 2 unotools/source/i18n/localedatawrapper.cxx | 4 unoxml/source/rdf/CLiteral.cxx | 2 unoxml/source/rdf/CURI.cxx | 2 vcl/unx/gtk/gtkinst.cxx | 4 16 files changed, 90 insertions(+), 60 deletions(-)
New commits: commit 7f3ca309513c8e7712378f05948ded3a34739d9d Author: Stephan Bergmann <[email protected]> Date: Tue Sep 12 18:24:46 2017 +0200 Enable -Wunreachable-code ...motivated by <https://gerrit.libreoffice.org/#/c/41565/2> adding dead code at the end of a switch statement, after the last case's "break". -Wunreachable-code appears to work well on Clang, while it appears to have no effect on GCC. Most of the affected places are apparently temporary/TODO/FIXME cases of disabling code via "if (false)", which can be written with an extra set of parentheses as "if ((false))" to silence -Wunreachable-code on Clang (which thus needed loplugin:unnecessaryparen to be adapted accordingly). In some cases, the controlling expression was more complex than just "false" and needed to be rewritten by taking it out of the if statement to silence Clang. One noteworthy case where the nature of the disabled code wasn't immediately apparent: Sep 12 16:59:58 <sberg> quikee, is that "if (false)" in ScExponentialSmoothingDialog::ApplyOutput (sc/source/ui/StatisticsDialogs/ExponentialSmoothingDialog.cxx) some work-in- progress or dead code? Sep 12 17:02:03 <quikee> sberg: WIP, but you can remove it Sep 12 17:04:47 <sberg> quikee, I'll wrap the false in an extra set of parentheses for now, to silence -Wunreachable-code (I wouldn't want to remove it, as I have no idea whether I should then also remove the "Initial value" comment preceding it) Sep 12 17:07:29 <quikee> sberg: both are different ways to calculate the "intital value"... so no Another case where the nature of the dead code, following while (true) loops without breaks, is unclear is sd/source/ui/remotecontrol/BluetoothServer.cxx, where I added TODO markers to the workarounds that silence the warnings for now. basic/source/sbx/sbxvalue.cxx had a variable of type double, of automatic storage duration, and without an initalizer at the top of a switch statement. Clang warning about it is arguably a false positive. Apart from that, this didn't find any cases of genuinely dead code in the existing code base. Change-Id: Ib00b822c8efec94278c048783d5997b8ba86a94c Reviewed-on: https://gerrit.libreoffice.org/42217 Tested-by: Stephan Bergmann <[email protected]> Reviewed-by: Stephan Bergmann <[email protected]> diff --git a/basic/source/sbx/sbxvalue.cxx b/basic/source/sbx/sbxvalue.cxx index fb52f2e3f0d1..0d5a1c432156 100644 --- a/basic/source/sbx/sbxvalue.cxx +++ b/basic/source/sbx/sbxvalue.cxx @@ -1062,73 +1062,80 @@ bool SbxValue::Compute( SbxOperator eOp, const SbxValue& rOp ) if( Get( aL ) ) switch( eOp ) { - double dTest; case SbxMUL: - // first overflow check: see if product will fit - test real value of product (hence 2 curr factors) - dTest = (double)aL.nInt64 * (double)aR.nInt64 / (double)CURRENCY_FACTOR_SQUARE; - if( dTest < SbxMINCURR || SbxMAXCURR < dTest) { - aL.nInt64 = SAL_MAX_INT64; - if( dTest < SbxMINCURR ) aL.nInt64 = SAL_MIN_INT64; - SetError( ERRCODE_BASIC_MATH_OVERFLOW ); - break; - } - // second overflow check: see if unscaled product overflows - if so use doubles - dTest = (double)aL.nInt64 * (double)aR.nInt64; - if( dTest < SAL_MIN_INT64 || SAL_MAX_INT64 < dTest) - { - aL.nInt64 = (sal_Int64)( dTest / (double)CURRENCY_FACTOR ); + // first overflow check: see if product will fit - test real value of product (hence 2 curr factors) + double dTest = (double)aL.nInt64 * (double)aR.nInt64 / (double)CURRENCY_FACTOR_SQUARE; + if( dTest < SbxMINCURR || SbxMAXCURR < dTest) + { + aL.nInt64 = SAL_MAX_INT64; + if( dTest < SbxMINCURR ) aL.nInt64 = SAL_MIN_INT64; + SetError( ERRCODE_BASIC_MATH_OVERFLOW ); + break; + } + // second overflow check: see if unscaled product overflows - if so use doubles + dTest = (double)aL.nInt64 * (double)aR.nInt64; + if( dTest < SAL_MIN_INT64 || SAL_MAX_INT64 < dTest) + { + aL.nInt64 = (sal_Int64)( dTest / (double)CURRENCY_FACTOR ); + break; + } + // precise calc: multiply then scale back (move decimal pt) + aL.nInt64 *= aR.nInt64; + aL.nInt64 /= CURRENCY_FACTOR; break; } - // precise calc: multiply then scale back (move decimal pt) - aL.nInt64 *= aR.nInt64; - aL.nInt64 /= CURRENCY_FACTOR; - break; case SbxDIV: - if( !aR.nInt64 ) { - SetError( ERRCODE_BASIC_ZERODIV ); - break; - } - // first overflow check: see if quotient will fit - calc real value of quotient (curr factors cancel) - dTest = (double)aL.nInt64 / (double)aR.nInt64; - if( dTest < SbxMINCURR || SbxMAXCURR < dTest) - { - SetError( ERRCODE_BASIC_MATH_OVERFLOW ); - break; - } - // second overflow check: see if scaled dividend overflows - if so use doubles - dTest = (double)aL.nInt64 * (double)CURRENCY_FACTOR; - if( dTest < SAL_MIN_INT64 || SAL_MAX_INT64 < dTest) - { - aL.nInt64 = (sal_Int64)(dTest / (double)aR.nInt64); + if( !aR.nInt64 ) + { + SetError( ERRCODE_BASIC_ZERODIV ); + break; + } + // first overflow check: see if quotient will fit - calc real value of quotient (curr factors cancel) + double dTest = (double)aL.nInt64 / (double)aR.nInt64; + if( dTest < SbxMINCURR || SbxMAXCURR < dTest) + { + SetError( ERRCODE_BASIC_MATH_OVERFLOW ); + break; + } + // second overflow check: see if scaled dividend overflows - if so use doubles + dTest = (double)aL.nInt64 * (double)CURRENCY_FACTOR; + if( dTest < SAL_MIN_INT64 || SAL_MAX_INT64 < dTest) + { + aL.nInt64 = (sal_Int64)(dTest / (double)aR.nInt64); + break; + } + // precise calc: scale (move decimal pt) then divide + aL.nInt64 *= CURRENCY_FACTOR; + aL.nInt64 /= aR.nInt64; break; } - // precise calc: scale (move decimal pt) then divide - aL.nInt64 *= CURRENCY_FACTOR; - aL.nInt64 /= aR.nInt64; - break; case SbxPLUS: - dTest = ( (double)aL.nInt64 + (double)aR.nInt64 ) / (double)CURRENCY_FACTOR; - if( dTest < SbxMINCURR || SbxMAXCURR < dTest) { - SetError( ERRCODE_BASIC_MATH_OVERFLOW ); + double dTest = ( (double)aL.nInt64 + (double)aR.nInt64 ) / (double)CURRENCY_FACTOR; + if( dTest < SbxMINCURR || SbxMAXCURR < dTest) + { + SetError( ERRCODE_BASIC_MATH_OVERFLOW ); + break; + } + aL.nInt64 += aR.nInt64; break; } - aL.nInt64 += aR.nInt64; - break; case SbxMINUS: - dTest = ( (double)aL.nInt64 - (double)aR.nInt64 ) / (double)CURRENCY_FACTOR; - if( dTest < SbxMINCURR || SbxMAXCURR < dTest) { - SetError( ERRCODE_BASIC_MATH_OVERFLOW ); + double dTest = ( (double)aL.nInt64 - (double)aR.nInt64 ) / (double)CURRENCY_FACTOR; + if( dTest < SbxMINCURR || SbxMAXCURR < dTest) + { + SetError( ERRCODE_BASIC_MATH_OVERFLOW ); + break; + } + aL.nInt64 -= aR.nInt64; break; } - aL.nInt64 -= aR.nInt64; - break; case SbxNEG: aL.nInt64 = -aL.nInt64; break; diff --git a/chart2/source/view/main/PropertyMapper.cxx b/chart2/source/view/main/PropertyMapper.cxx index 07bf987ec336..7f0e5d5c0477 100644 --- a/chart2/source/view/main/PropertyMapper.cxx +++ b/chart2/source/view/main/PropertyMapper.cxx @@ -82,7 +82,7 @@ void PropertyMapper::getValueMap( tPropertyNameMap::const_iterator aEnd( rNameMap.end() ); uno::Reference< beans::XMultiPropertySet > xMultiPropSet(xSourceProp, uno::UNO_QUERY); - if(false && xMultiPropSet.is()) + if((false) && xMultiPropSet.is()) { uno::Sequence< rtl::OUString > aPropSourceNames(rNameMap.size()); uno::Sequence< rtl::OUString > aPropTargetNames(rNameMap.size()); diff --git a/compilerplugins/clang/test/unnecessaryparen.cxx b/compilerplugins/clang/test/unnecessaryparen.cxx index cb237c551889..51f792769bc2 100644 --- a/compilerplugins/clang/test/unnecessaryparen.cxx +++ b/compilerplugins/clang/test/unnecessaryparen.cxx @@ -34,6 +34,12 @@ int main() int v1 = (static_cast<short>(1)) + 1; // expected-error {{unnecessary parentheses around cast [loplugin:unnecessaryparen]}} (void)v1; + + // No warnings, used to silence -Wunreachable-code: + if ((false)) { + return 0; + } + x = (true) ? 0 : 1; }; /* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s cinkeys+=0=break: */ diff --git a/compilerplugins/clang/test/vclwidgets.cxx b/compilerplugins/clang/test/vclwidgets.cxx index 9ead1c905289..c470f991a667 100644 --- a/compilerplugins/clang/test/vclwidgets.cxx +++ b/compilerplugins/clang/test/vclwidgets.cxx @@ -22,7 +22,7 @@ struct Widget : public VclReferenceBase Widget* p = mpParent; (void)p; // test against false+ - p = true ? mpParent.get() : nullptr; + p = (true) ? mpParent.get() : nullptr; } ~Widget() override diff --git a/compilerplugins/clang/unnecessaryparen.cxx b/compilerplugins/clang/unnecessaryparen.cxx index 8a94051d5bf4..02b71694e6ac 100644 --- a/compilerplugins/clang/unnecessaryparen.cxx +++ b/compilerplugins/clang/unnecessaryparen.cxx @@ -224,6 +224,12 @@ void UnnecessaryParen::VisitSomeStmt(const Stmt *parent, const Expr* cond, Strin if (parenExpr) { if (parenExpr->getLocStart().isMacroID()) return; + // Used to silence -Wunreachable-code: + if (isa<CXXBoolLiteralExpr>(parenExpr->getSubExpr()) + && stmtName == "if") + { + return; + } // assignments need extra parentheses or they generate a compiler warning auto binaryOp = dyn_cast<BinaryOperator>(parenExpr->getSubExpr()); if (binaryOp && binaryOp->getOpcode() == BO_Assign) diff --git a/cppu/qa/test_unotype.cxx b/cppu/qa/test_unotype.cxx index 391e9b352962..32f6db263c6e 100644 --- a/cppu/qa/test_unotype.cxx +++ b/cppu/qa/test_unotype.cxx @@ -94,7 +94,7 @@ public: void Test::testUnoType() { // Avoid warnings about unused ~DerivedInterface1/2 (see above): - if (false) { + if ((false)) { DerivedInterface1::dummy(nullptr); DerivedInterface2::dummy(nullptr); } diff --git a/cui/source/dialogs/about.cxx b/cui/source/dialogs/about.cxx index d5f0d2eabe89..6a4baa7ce75b 100644 --- a/cui/source/dialogs/about.cxx +++ b/cui/source/dialogs/about.cxx @@ -316,7 +316,9 @@ OUString AboutDialog::GetVersionString() sVersion += "\n" + Application::GetHWOSConfInfo(); - if (EXTRA_BUILDID[0] != '\0') + bool const extra = EXTRA_BUILDID[0] != '\0'; + // extracted from the 'if' to avoid Clang -Wunreachable-code + if (extra) { sVersion += "\n" EXTRA_BUILDID; } diff --git a/sc/source/ui/StatisticsDialogs/ExponentialSmoothingDialog.cxx b/sc/source/ui/StatisticsDialogs/ExponentialSmoothingDialog.cxx index 2bad4e9e702a..35c8a3b077a7 100644 --- a/sc/source/ui/StatisticsDialogs/ExponentialSmoothingDialog.cxx +++ b/sc/source/ui/StatisticsDialogs/ExponentialSmoothingDialog.cxx @@ -99,7 +99,7 @@ ScRange ScExponentialSmoothingDialog::ApplyOutput(ScDocShell* pDocShell) output.nextRow(); // Initial value - if (false) + if ((false)) { aTemplate.setTemplate("=AVERAGE(%RANGE%)"); aTemplate.applyRange("%RANGE%", aCurrentRange); diff --git a/sd/source/ui/remotecontrol/BluetoothServer.cxx b/sd/source/ui/remotecontrol/BluetoothServer.cxx index 2cc8380926ca..a4083c3625b5 100644 --- a/sd/source/ui/remotecontrol/BluetoothServer.cxx +++ b/sd/source/ui/remotecontrol/BluetoothServer.cxx @@ -1207,6 +1207,9 @@ void SAL_CALL BluetoothServer::run() while (DBUS_DISPATCH_DATA_REMAINS == dbus_connection_get_dispatch_status( pConnection )) dbus_connection_dispatch( pConnection ); } + if ((false)) break; + // silence Clang -Wunreachable-code after loop (TODO: proper + // fix?) } unregisterBluez5Profile( pConnection ); g_main_context_unref( mpImpl->mpContext ); @@ -1295,6 +1298,8 @@ void SAL_CALL BluetoothServer::run() pCommunicator->launch(); } } + if ((false)) break; + // silence Clang -Wunreachable-code after loop (TODO: proper fix?) } unregisterBluez5Profile( pConnection ); diff --git a/solenv/gbuild/platform/com_GCC_defs.mk b/solenv/gbuild/platform/com_GCC_defs.mk index 3cecb8286311..977aa0c1eac1 100644 --- a/solenv/gbuild/platform/com_GCC_defs.mk +++ b/solenv/gbuild/platform/com_GCC_defs.mk @@ -54,6 +54,7 @@ gb_CFLAGS_COMMON := \ -Wextra \ -Wstrict-prototypes \ -Wundef \ + -Wunreachable-code \ -Wunused-macros \ -finput-charset=UTF-8 \ -fmessage-length=0 \ @@ -67,6 +68,7 @@ gb_CXXFLAGS_COMMON := \ -Wendif-labels \ -Wextra \ -Wundef \ + -Wunreachable-code \ -Wunused-macros \ -finput-charset=UTF-8 \ -fmessage-length=0 \ diff --git a/sw/source/core/doc/tblrwcl.cxx b/sw/source/core/doc/tblrwcl.cxx index d8089483435a..45c0e9d1de0e 100644 --- a/sw/source/core/doc/tblrwcl.cxx +++ b/sw/source/core/doc/tblrwcl.cxx @@ -3985,7 +3985,7 @@ static bool lcl_SetOtherLineHeight( SwTableLine* pLine, CR_SetLineHeight& rParam // Calculate the new relative size by means of the old one // If the selected Box get bigger, adjust via the max space else // via the max height. - if( true /*!rParam.bBigger*/ ) + if( (true) /*!rParam.bBigger*/ ) { nDist *= pLineFrame->Frame().Height(); nDist /= rParam.nMaxHeight; diff --git a/sw/source/core/undo/undobj1.cxx b/sw/source/core/undo/undobj1.cxx index e17bc136d832..0d8249567e73 100644 --- a/sw/source/core/undo/undobj1.cxx +++ b/sw/source/core/undo/undobj1.cxx @@ -345,7 +345,7 @@ OUString SwUndoInsLayFormat::GetComment() const // have a SwDrawContact yet, so it will fall back to SwUndo::GetComment(), // which sets pComment to a wrong value. // if (! pComment) - if (true) + if ((true)) { /* If frame format is present and has an SdrObject use the undo diff --git a/unotools/source/i18n/localedatawrapper.cxx b/unotools/source/i18n/localedatawrapper.cxx index 69cb9c066f49..1750a9b23bc5 100644 --- a/unotools/source/i18n/localedatawrapper.cxx +++ b/unotools/source/i18n/localedatawrapper.cxx @@ -1376,7 +1376,7 @@ OUString LocaleDataWrapper::getDate( const Date& rDate ) const sal_Int16 nYear = rDate.GetYear(); sal_uInt16 nYearLen; - if ( true /* IsDateCentury() */ ) + if ( (true) /* IsDateCentury() */ ) nYearLen = 4; else { @@ -1490,7 +1490,7 @@ OUString LocaleDataWrapper::getDuration( const tools::Time& rTime, bool bSec, bo if ( rTime < tools::Time( 0 ) ) pBuf = ImplAddString( pBuf, ' ' ); - if ( true /* IsTimeLeadingZero() */ ) + if ( (true) /* IsTimeLeadingZero() */ ) pBuf = ImplAddUNum( pBuf, rTime.GetHour(), 2 ); else pBuf = ImplAddUNum( pBuf, rTime.GetHour() ); diff --git a/unoxml/source/rdf/CLiteral.cxx b/unoxml/source/rdf/CLiteral.cxx index 7093ee76552d..e1ffabccfc94 100644 --- a/unoxml/source/rdf/CLiteral.cxx +++ b/unoxml/source/rdf/CLiteral.cxx @@ -102,7 +102,7 @@ void SAL_CALL CLiteral::initialize(const css::uno::Sequence< css::uno::Any > & a "CLiteral::initialize: argument must be string", *this, 0); } //FIXME: what is legal? - if (true) { + if ((true)) { m_Value = arg0; } else { throw css::lang::IllegalArgumentException( diff --git a/unoxml/source/rdf/CURI.cxx b/unoxml/source/rdf/CURI.cxx index 35471873ff66..4ce0741f5cda 100644 --- a/unoxml/source/rdf/CURI.cxx +++ b/unoxml/source/rdf/CURI.cxx @@ -765,7 +765,7 @@ void SAL_CALL CURI::initialize(const css::uno::Sequence< css::uno::Any > & aArgu "CURI::initialize: argument is not valid namespace", *this, 0); } //FIXME: what is legal? - if (true) { + if ((true)) { m_LocalName = arg1; } else { throw css::lang::IllegalArgumentException( diff --git a/vcl/unx/gtk/gtkinst.cxx b/vcl/unx/gtk/gtkinst.cxx index db0aa9b783e4..8a45c62abc8b 100644 --- a/vcl/unx/gtk/gtkinst.cxx +++ b/vcl/unx/gtk/gtkinst.cxx @@ -99,7 +99,9 @@ extern "C" GtkYieldMutex *pYieldMutex; // init gdk thread protection - if ( !g_thread_supported() ) + bool const sup = g_thread_supported(); + // extracted from the 'if' to avoid Clang -Wunreachable-code + if ( !sup ) g_thread_init( nullptr ); gdk_threads_set_lock_functions (GdkThreadsEnter, GdkThreadsLeave); _______________________________________________ Libreoffice-commits mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/libreoffice-commits
