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>

6 files changed, 27 insertions, 23 deletions

diff --git a/toolkit/source/awt/animatedimagespeer.cxx b/toolkit/source/awt/animatedimagespeer.cxx
index e056ef04d55c..e833f366aa21 100644
--- a/toolkit/source/awt/animatedimagespeer.cxx
+++ b/toolkit/source/awt/animatedimagespeer.cxx
@@ -31,6 +31,7 @@
 
 #include <comphelper/namedvaluecollection.hxx>
 #include <comphelper/processfactory.hxx>
+#include <o3tl/safeint.hxx>
 #include <rtl/ustrbuf.hxx>
 #include <tools/diagnose_ex.h>
 #include <tools/urlobj.hxx>
@@ -245,7 +246,7 @@ namespace toolkit
 
                 // found a set?
                 std::vector< Image > aImages;
-                if ( ( nPreferredSet >= 0 ) && ( size_t( nPreferredSet ) < nImageSetCount ) )
+                if ( ( nPreferredSet >= 0 ) && ( o3tl::make_unsigned( nPreferredSet ) < nImageSetCount ) )
                 {
                     // => set the images
                     ::std::vector< CachedImage > const& rImageSet( i_data.aCachedImageSets[ nPreferredSet ] );
diff --git a/toolkit/source/controls/animatedimages.cxx b/toolkit/source/controls/animatedimages.cxx
index efa48cec0b1f..d35224d3ac93 100644
--- a/toolkit/source/controls/animatedimages.cxx
+++ b/toolkit/source/controls/animatedimages.cxx
@@ -31,7 +31,7 @@
 #include <com/sun/star/container/XContainerListener.hpp>
 #include <com/sun/star/uno/XComponentContext.hpp>
 #include <com/sun/star/util/XModifyListener.hpp>
-
+#include <o3tl/safeint.hxx>
 #include <toolkit/controls/unocontrolbase.hxx>
 #include <toolkit/controls/unocontrolmodel.hxx>
 
@@ -213,7 +213,7 @@ namespace toolkit {
         void lcl_checkIndex( const AnimatedImagesControlModel_Data& i_data, const sal_Int32 i_index, const Reference< XInterface >& i_context,
             const bool i_forInsert = false )
         {
-            if ( ( i_index < 0 ) || ( size_t( i_index ) > i_data.aImageSets.size() + ( i_forInsert ? 1 : 0 ) ) )
+            if ( ( i_index < 0 ) || ( o3tl::make_unsigned( i_index ) > i_data.aImageSets.size() + ( i_forInsert ? 1 : 0 ) ) )
                 throw IndexOutOfBoundsException( OUString(), i_context );
         }
 
diff --git a/toolkit/source/controls/grid/defaultgridcolumnmodel.cxx b/toolkit/source/controls/grid/defaultgridcolumnmodel.cxx
index 9d6a28fa6625..8b97158d0df0 100644
--- a/toolkit/source/controls/grid/defaultgridcolumnmodel.cxx
+++ b/toolkit/source/controls/grid/defaultgridcolumnmodel.cxx
@@ -33,6 +33,7 @@
 #include <cppuhelper/basemutex.hxx>
 #include <cppuhelper/compbase.hxx>
 #include <cppuhelper/supportsservice.hxx>
+#include <o3tl/safeint.hxx>
 #include <rtl/ref.hxx>
 #include <rtl/ustrbuf.hxx>
 #include <sal/log.hxx>
@@ -177,7 +178,7 @@ private:
     {
         ::comphelper::ComponentGuard aGuard( *this, rBHelper );
 
-        if ( ( i_columnIndex < 0 ) || ( size_t( i_columnIndex ) >= m_aColumns.size() ) )
+        if ( ( i_columnIndex < 0 ) || ( o3tl::make_unsigned( i_columnIndex ) >= m_aColumns.size() ) )
             throw css::lang::IndexOutOfBoundsException( OUString(), *this );
 
         Columns::iterator const pos = m_aColumns.begin() + i_columnIndex;
diff --git a/toolkit/source/controls/grid/defaultgriddatamodel.cxx b/toolkit/source/controls/grid/defaultgriddatamodel.cxx
index e29315ddd79c..1945204c559b 100644
--- a/toolkit/source/controls/grid/defaultgriddatamodel.cxx
+++ b/toolkit/source/controls/grid/defaultgriddatamodel.cxx
@@ -27,6 +27,7 @@
 #include <cppuhelper/basemutex.hxx>
 #include <cppuhelper/compbase.hxx>
 #include <cppuhelper/supportsservice.hxx>
+#include <o3tl/safeint.hxx>
 
 #include <algorithm>
 #include <vector>
@@ -153,13 +154,13 @@ private:
 
     DefaultGridDataModel::CellData const & DefaultGridDataModel::impl_getCellData_throw( sal_Int32 const i_column, sal_Int32 const i_row ) const
     {
-        if  (   ( i_row < 0 ) || ( size_t( i_row ) > m_aData.size() )
+        if  (   ( i_row < 0 ) || ( o3tl::make_unsigned( i_row ) > m_aData.size() )
             ||  ( i_column < 0 ) || ( i_column > m_nColumnCount )
             )
             throw IndexOutOfBoundsException( OUString(), *const_cast< DefaultGridDataModel* >( this ) );
 
         RowData const & rRow( m_aData[ i_row ] );
-        if ( size_t( i_column ) < rRow.size() )
+        if ( o3tl::make_unsigned( i_column ) < rRow.size() )
             return rRow[ i_column ];
 
         static CellData s_aEmpty;
@@ -169,8 +170,8 @@ private:
 
     DefaultGridDataModel::RowData& DefaultGridDataModel::impl_getRowDataAccess_throw( sal_Int32 const i_rowIndex, size_t const i_requiredColumnCount )
     {
-        OSL_ENSURE( i_requiredColumnCount <= size_t( m_nColumnCount ), "DefaultGridDataModel::impl_getRowDataAccess_throw: invalid column count!" );
-        if  ( ( i_rowIndex < 0 ) || ( size_t( i_rowIndex ) >= m_aData.size() ) )
+        OSL_ENSURE( i_requiredColumnCount <= o3tl::make_unsigned( m_nColumnCount ), "DefaultGridDataModel::impl_getRowDataAccess_throw: invalid column count!" );
+        if  ( ( i_rowIndex < 0 ) || ( o3tl::make_unsigned( i_rowIndex ) >= m_aData.size() ) )
             throw IndexOutOfBoundsException( OUString(), *this );
 
         RowData& rRowData( m_aData[ i_rowIndex ] );
@@ -208,7 +209,7 @@ private:
     {
         ::comphelper::ComponentGuard aGuard( *this, rBHelper );
 
-        if ( ( i_row < 0 ) || ( size_t( i_row ) >= m_aRowHeaders.size() ) )
+        if ( ( i_row < 0 ) || ( o3tl::make_unsigned( i_row ) >= m_aRowHeaders.size() ) )
             throw IndexOutOfBoundsException( OUString(), *this );
 
         return m_aRowHeaders[ i_row ];
@@ -328,7 +329,7 @@ private:
     {
         ::comphelper::ComponentGuard aGuard( *this, rBHelper );
 
-        if ( ( i_rowIndex < 0 ) || ( size_t( i_rowIndex ) >= m_aData.size() ) )
+        if ( ( i_rowIndex < 0 ) || ( o3tl::make_unsigned( i_rowIndex ) >= m_aData.size() ) )
             throw IndexOutOfBoundsException( OUString(), *this );
 
         m_aRowHeaders.erase( m_aRowHeaders.begin() + i_rowIndex );
@@ -375,7 +376,7 @@ private:
     {
         ::comphelper::ComponentGuard aGuard( *this, rBHelper );
 
-        if  ( ( i_rowIndex < 0 ) || ( size_t( i_rowIndex ) >= m_aData.size() ) )
+        if  ( ( i_rowIndex < 0 ) || ( o3tl::make_unsigned( i_rowIndex ) >= m_aData.size() ) )
             throw IndexOutOfBoundsException( OUString(), *this );
 
         if ( i_columnIndexes.getLength() != i_values.getLength() )
@@ -395,7 +396,7 @@ private:
         for ( sal_Int32 col = 0; col < columnCount; ++col )
         {
             sal_Int32 const columnIndex = i_columnIndexes[ col ];
-            if ( size_t( columnIndex ) >= rDataRow.size() )
+            if ( o3tl::make_unsigned( columnIndex ) >= rDataRow.size() )
                 rDataRow.resize( columnIndex + 1 );
 
             rDataRow[ columnIndex ].first = i_values[ col ];
@@ -416,7 +417,7 @@ private:
     {
         ::comphelper::ComponentGuard aGuard( *this, rBHelper );
 
-        if  ( ( i_rowIndex < 0 ) || ( size_t( i_rowIndex ) >= m_aRowHeaders.size() ) )
+        if  ( ( i_rowIndex < 0 ) || ( o3tl::make_unsigned( i_rowIndex ) >= m_aRowHeaders.size() ) )
             throw IndexOutOfBoundsException( OUString(), *this );
 
         m_aRowHeaders[ i_rowIndex ] = i_heading;
diff --git a/toolkit/source/controls/grid/sortablegriddatamodel.cxx b/toolkit/source/controls/grid/sortablegriddatamodel.cxx
index 711c7c23fa8d..ffbb6528dc62 100644
--- a/toolkit/source/controls/grid/sortablegriddatamodel.cxx
+++ b/toolkit/source/controls/grid/sortablegriddatamodel.cxx
@@ -38,6 +38,7 @@
 #include <cppuhelper/supportsservice.hxx>
 #include <tools/diagnose_ex.h>
 #include <i18nlangtag/languagetag.hxx>
+#include <o3tl/safeint.hxx>
 #include <vcl/svapp.hxx>
 #include <vcl/settings.hxx>
 
@@ -437,7 +438,7 @@ void lcl_clear( STLCONTAINER& i_container )
             OSL_ENSURE( false, "SortableGridDataModel::rowsRemoved: missing implementation - removal of multiple rows!" );
             needReIndex = true;
         }
-        else if ( size_t( i_event.FirstRow ) >= m_privateToPublicRowIndex.size() )
+        else if ( o3tl::make_unsigned( i_event.FirstRow ) >= m_privateToPublicRowIndex.size() )
         {
             OSL_ENSURE( false, "SortableGridDataModel::rowsRemoved: inconsistent/wrong data!" );
             needReIndex = true;
@@ -891,7 +892,7 @@ void lcl_clear( STLCONTAINER& i_container )
             // no need to translate anything
             return i_publicRowIndex;
 
-        ENSURE_OR_RETURN( size_t( i_publicRowIndex ) < m_publicToPrivateRowIndex.size(),
+        ENSURE_OR_RETURN( o3tl::make_unsigned( i_publicRowIndex ) < m_publicToPrivateRowIndex.size(),
             "SortableGridDataModel::impl_getPrivateRowIndex_throw: inconsistency!", i_publicRowIndex );
                 // obviously the translation table contains too few elements - it should have exactly |getRowCount()|
                 // elements
@@ -909,7 +910,7 @@ void lcl_clear( STLCONTAINER& i_container )
         if ( i_privateRowIndex < 0 )
             return i_privateRowIndex;
 
-        ENSURE_OR_RETURN( size_t( i_privateRowIndex ) < m_privateToPublicRowIndex.size(),
+        ENSURE_OR_RETURN( o3tl::make_unsigned( i_privateRowIndex ) < m_privateToPublicRowIndex.size(),
             "SortableGridDataModel::impl_getPublicRowIndex_nothrow: invalid index!", i_privateRowIndex );
 
         return m_privateToPublicRowIndex[ i_privateRowIndex ];
diff --git a/toolkit/source/controls/unocontrols.cxx b/toolkit/source/controls/unocontrols.cxx
index 81eab576bd30..b80d89f2d01b 100644
--- a/toolkit/source/controls/unocontrols.cxx
+++ b/toolkit/source/controls/unocontrols.cxx
@@ -28,7 +28,7 @@
 #include <com/sun/star/util/Date.hpp>
 #include <com/sun/star/awt/ImageScaleMode.hpp>
 
-
+#include <o3tl/safeint.hxx>
 #include <toolkit/controls/formattedcontrol.hxx>
 #include <toolkit/controls/unocontrols.hxx>
 #include <toolkit/helper/property.hxx>
@@ -2417,8 +2417,8 @@ void UnoControlListBoxModel::impl_handleInsert( const sal_Int32 i_nItemPosition,
     // sync with legacy StringItemList property
     ::std::vector< OUString > aStringItems;
     impl_getStringItemList( aStringItems );
-    OSL_ENSURE( size_t( i_nItemPosition ) <= aStringItems.size(), "UnoControlListBoxModel::impl_handleInsert" );
-    if ( size_t( i_nItemPosition ) <= aStringItems.size() )
+    OSL_ENSURE( o3tl::make_unsigned( i_nItemPosition ) <= aStringItems.size(), "UnoControlListBoxModel::impl_handleInsert" );
+    if ( o3tl::make_unsigned( i_nItemPosition ) <= aStringItems.size() )
     {
         const OUString sItemText( !!i_rItemText ? *i_rItemText : OUString() );
         aStringItems.insert( aStringItems.begin() + i_nItemPosition, sItemText );
@@ -2442,8 +2442,8 @@ void UnoControlListBoxModel::impl_handleRemove( const sal_Int32 i_nItemPosition,
     impl_getStringItemList( aStringItems );
     if ( !bAllItems )
     {
-        OSL_ENSURE( size_t( i_nItemPosition ) < aStringItems.size(), "UnoControlListBoxModel::impl_handleRemove" );
-        if ( size_t( i_nItemPosition ) < aStringItems.size() )
+        OSL_ENSURE( o3tl::make_unsigned( i_nItemPosition ) < aStringItems.size(), "UnoControlListBoxModel::impl_handleRemove" );
+        if ( o3tl::make_unsigned( i_nItemPosition ) < aStringItems.size() )
         {
             aStringItems.erase( aStringItems.begin() + i_nItemPosition );
         }
@@ -2480,8 +2480,8 @@ void UnoControlListBoxModel::impl_handleModify( const sal_Int32 i_nItemPosition,
         // sync with legacy StringItemList property
         ::std::vector< OUString > aStringItems;
         impl_getStringItemList( aStringItems );
-        OSL_ENSURE( size_t( i_nItemPosition ) < aStringItems.size(), "UnoControlListBoxModel::impl_handleModify" );
-        if ( size_t( i_nItemPosition ) < aStringItems.size() )
+        OSL_ENSURE( o3tl::make_unsigned( i_nItemPosition ) < aStringItems.size(), "UnoControlListBoxModel::impl_handleModify" );
+        if ( o3tl::make_unsigned( i_nItemPosition ) < aStringItems.size() )
         {
             aStringItems[ i_nItemPosition] = *i_rItemText;
         }