New loplugin:unsignedcompare

"Find explicit casts from signed to unsigned integer in comparison against
unsigned integer, where the cast is presumably used to avoid warnings about
signed vs. unsigned comparisons, and could thus be replaced with
o3tl::make_unsigned for clairty." (compilerplugins/clang/unsignedcompare.cxx)

o3tl::make_unsigned requires its argument to be non-negative, and there is a
chance that some original code like

static_cast<sal_uInt32>(n) >= c

used the explicit cast to actually force a (potentially negative) value of
sal_Int32 to be interpreted as an unsigned sal_uInt32, rather than using the
cast to avoid a false "signed vs. unsigned comparison" warning in a case where
n is known to be non-negative.  It appears that restricting this plugin to non-
equality comparisons (<, >, <=, >=) and excluding equality comparisons (==, !=)
is a useful heuristic to avoid such false positives.  The only remainging false
positive I found was 0288c8ffecff4956a52b9147d441979941e8b87f "Rephrase cast
from sal_Int32 to sal_uInt32".

But which of course does not mean that there were no further false positivies
that I missed.  So this commit may accidentally introduce some false hits of the
assert in o3tl::make_unsigned.  At least, it passed a full (Linux ASan+UBSan
--enable-dbgutil) `make check && make screenshot`.

It is by design that o3tl::make_unsigned only accepts signed integer parameter
types (and is not defined as a nop for unsigned ones), to avoid unnecessary uses
which would in general be suspicious.  But the STATIC_ARRAY_SELECT macro in
include/oox/helper/helper.hxx is used with both signed and unsigned types, so
needs a little oox::detail::make_unsigned helper function for now.  (The
ultimate fix being to get rid of the macro in the first place.)

Change-Id: Ia4adc9f44c70ad1dfd608784cac39ee922c32175
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/87556
Tested-by: Jenkins
Reviewed-by: Stephan Bergmann <sbergman@redhat.com>

5 files changed, 16 insertions, 11 deletions

diff --git a/svtools/source/brwbox/brwbox1.cxx b/svtools/source/brwbox/brwbox1.cxx
index 2aead90c8d04..e9be5c9bc4a3 100644
--- a/svtools/source/brwbox/brwbox1.cxx
+++ b/svtools/source/brwbox/brwbox1.cxx
@@ -20,6 +20,7 @@
 #include <svtools/brwbox.hxx>
 #include <svtools/brwhead.hxx>
 #include <o3tl/numeric.hxx>
+#include <o3tl/safeint.hxx>
 #include "datwin.hxx"
 #include <tools/debug.hxx>
 #include <tools/fract.hxx>
@@ -523,7 +524,7 @@ void BrowseBox::SetColumnWidth( sal_uInt16 nItemId, sal_uLong nWidth )
         nMaxWidth -= pDataWin->bAutoSizeLastCol
                 ? GetFieldRect(nItemId).Left()
                 : GetFrozenWidth();
-        if ( pDataWin->bAutoSizeLastCol || nWidth > static_cast<sal_uLong>(nMaxWidth) )
+        if ( pDataWin->bAutoSizeLastCol || nWidth > o3tl::make_unsigned(nMaxWidth) )
         {
             nWidth = nMaxWidth > 16 ? nMaxWidth : nOldWidth;
         }
diff --git a/svtools/source/brwbox/brwbox2.cxx b/svtools/source/brwbox/brwbox2.cxx
index 8daede5fa7b9..877277701e14 100644
--- a/svtools/source/brwbox/brwbox2.cxx
+++ b/svtools/source/brwbox/brwbox2.cxx
@@ -804,7 +804,7 @@ void BrowseBox::ImplPaintData(OutputDevice& _rOut, const tools::Rectangle& _rRec
 
     // redraw the invalid fields
     for ( sal_uLong nRelRow = nRelTopRow;
-          nRelRow <= nRelBottomRow && static_cast<sal_uLong>(nTopRow)+nRelRow < static_cast<sal_uLong>(nRowCount);
+          nRelRow <= nRelBottomRow && static_cast<sal_uLong>(nTopRow)+nRelRow < o3tl::make_unsigned(nRowCount);
           ++nRelRow, aPos.AdjustY(nDataRowHeigt ) )
     {
         // get row
diff --git a/svtools/source/control/valueset.cxx b/svtools/source/control/valueset.cxx
index 4c5fb740508c..dce387faf942 100644
--- a/svtools/source/control/valueset.cxx
+++ b/svtools/source/control/valueset.cxx
@@ -17,6 +17,9 @@
  *   the License at http://www.apache.org/licenses/LICENSE-2.0 .
  */
 
+#include <sal/config.h>
+
+#include <o3tl/safeint.hxx>
 #include <tools/debug.hxx>
 #include <tools/stream.hxx>
 #include <comphelper/base64.hxx>
@@ -456,7 +459,7 @@ void ValueSet::Format(vcl::RenderContext& rRenderContext)
     }
     else
     {
-        if (mnFirstLine > static_cast<sal_uInt16>(mnLines - mnVisLines))
+        if (mnFirstLine > o3tl::make_unsigned(mnLines - mnVisLines))
             mnFirstLine = static_cast<sal_uInt16>(mnLines - mnVisLines);
     }
 
@@ -903,7 +906,7 @@ bool ValueSet::ImplScroll(const Point& rPos)
     }
     else if (rPos.Y() >= maItemListRect.Bottom() - nScrollOffset)
     {
-        if (mnFirstLine < static_cast<sal_uInt16>(mnLines - mnVisLines))
+        if (mnFirstLine < o3tl::make_unsigned(mnLines - mnVisLines))
         {
             ++mnFirstLine;
             bScroll = true;
@@ -1737,7 +1740,7 @@ void ValueSet::SelectItem( sal_uInt16 nItemId )
             mnFirstLine = nNewLine;
             bNewLine = true;
         }
-        else if ( nNewLine > static_cast<sal_uInt16>(mnFirstLine+mnVisLines-1) )
+        else if ( nNewLine > o3tl::make_unsigned(mnFirstLine+mnVisLines-1) )
         {
             mnFirstLine = static_cast<sal_uInt16>(nNewLine-mnVisLines+1);
             bNewLine = true;
@@ -2825,7 +2828,7 @@ void SvtValueSet::SelectItem( sal_uInt16 nItemId )
             mnFirstLine = nNewLine;
             bNewLine = true;
         }
-        else if ( nNewLine > static_cast<sal_uInt16>(mnFirstLine+mnVisLines-1) )
+        else if ( nNewLine > o3tl::make_unsigned(mnFirstLine+mnVisLines-1) )
         {
             mnFirstLine = static_cast<sal_uInt16>(nNewLine-mnVisLines+1);
             bNewLine = true;
@@ -3019,7 +3022,7 @@ void SvtValueSet::Format(vcl::RenderContext const & rRenderContext)
     }
     else
     {
-        if (mnFirstLine > static_cast<sal_uInt16>(mnLines - mnVisLines))
+        if (mnFirstLine > o3tl::make_unsigned(mnLines - mnVisLines))
             mnFirstLine = static_cast<sal_uInt16>(mnLines - mnVisLines);
     }
 
diff --git a/svtools/source/dialogs/ServerDetailsControls.cxx b/svtools/source/dialogs/ServerDetailsControls.cxx
index bfbc2e21593b..0f95e7fc3d40 100644
--- a/svtools/source/dialogs/ServerDetailsControls.cxx
+++ b/svtools/source/dialogs/ServerDetailsControls.cxx
@@ -17,6 +17,7 @@
 #include <com/sun/star/sdbc/XRow.hpp>
 
 #include <comphelper/processfactory.hxx>
+#include <o3tl/safeint.hxx>
 #include <rtl/uri.hxx>
 #include <ucbhelper/content.hxx>
 #include <ucbhelper/commandenvironment.hxx>
@@ -385,7 +386,7 @@ void CmisDetailsContainer::selectRepository( )
 {
     // Get the repo ID and call the Change listener
     const int nPos = m_pDialog->m_xLBRepository->get_active();
-    if( static_cast<size_t>(nPos) < m_aRepoIds.size() )
+    if( o3tl::make_unsigned(nPos) < m_aRepoIds.size() )
     {
         m_sRepoId = m_aRepoIds[nPos];
         notifyChange( );
diff --git a/svtools/source/uno/unocontroltablemodel.cxx b/svtools/source/uno/unocontroltablemodel.cxx
index 12b48d129e33..32785a32fd8d 100644
--- a/svtools/source/uno/unocontroltablemodel.cxx
+++ b/svtools/source/uno/unocontroltablemodel.cxx
@@ -26,7 +26,7 @@
 #include <com/sun/star/awt/grid/XGridColumn.hpp>
 #include <com/sun/star/awt/grid/XSortableGridData.hpp>
 #include <com/sun/star/util/Color.hpp>
-
+#include <o3tl/safeint.hxx>
 #include <sal/log.hxx>
 #include <cppuhelper/weakref.hxx>
 #include <tools/debug.hxx>
@@ -214,7 +214,7 @@ namespace svt { namespace table
     void UnoControlTableModel::insertColumn( ColPos const i_position, Reference< XGridColumn > const & i_column )
     {
         DBG_CHECK_ME();
-        ENSURE_OR_RETURN_VOID( ( i_position >= 0 ) && ( size_t( i_position ) <= m_pImpl->aColumns.size() ),
+        ENSURE_OR_RETURN_VOID( ( i_position >= 0 ) && ( o3tl::make_unsigned( i_position ) <= m_pImpl->aColumns.size() ),
             "UnoControlTableModel::insertColumn: illegal position!" );
 
         const PColumnModel pColumn = std::make_shared<UnoGridColumnFacade>( *this, i_column );
@@ -232,7 +232,7 @@ namespace svt { namespace table
     void UnoControlTableModel::removeColumn( ColPos const i_position )
     {
         DBG_CHECK_ME();
-        ENSURE_OR_RETURN_VOID( ( i_position >= 0 ) && ( size_t( i_position ) <= m_pImpl->aColumns.size() ),
+        ENSURE_OR_RETURN_VOID( ( i_position >= 0 ) && ( o3tl::make_unsigned( i_position ) <= m_pImpl->aColumns.size() ),
             "UnoControlTableModel::removeColumn: illegal position!" );
 
         // remove the column