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>

8 files changed, 22 insertions, 14 deletions

diff --git a/chart2/source/controller/accessibility/AccessibleBase.cxx b/chart2/source/controller/accessibility/AccessibleBase.cxx
index 26657d429bd1..840f578aa17a 100644
--- a/chart2/source/controller/accessibility/AccessibleBase.cxx
+++ b/chart2/source/controller/accessibility/AccessibleBase.cxx
@@ -42,6 +42,7 @@
 #include <vcl/window.hxx>
 #include <vcl/settings.hxx>
 #include <o3tl/functional.hxx>
+#include <o3tl/safeint.hxx>
 #include <tools/diagnose_ex.h>
 #include <unotools/accessiblestatesethelper.hxx>
 
@@ -503,7 +504,7 @@ Reference< XAccessible > AccessibleBase::ImplGetAccessibleChildById( sal_Int32 i
     MutexGuard aGuard( m_aMutex);
     if( ! m_bMayHaveChildren ||
         i < 0 ||
-        static_cast< ChildListVectorType::size_type >( i ) >= m_aChildList.size() )
+        o3tl::make_unsigned( i ) >= m_aChildList.size() )
     {
         OUString aBuf = "Index " + OUString::number( i ) + " is invalid for range [ 0, " +
                         OUString::number( m_aChildList.size() - 1 ) +
diff --git a/chart2/source/controller/dialogs/DataBrowser.cxx b/chart2/source/controller/dialogs/DataBrowser.cxx
index cbfb03b50031..465496f62979 100644
--- a/chart2/source/controller/dialogs/DataBrowser.cxx
+++ b/chart2/source/controller/dialogs/DataBrowser.cxx
@@ -36,6 +36,7 @@
 #include <vcl/svapp.hxx>
 #include <vcl/virdev.hxx>
 #include <rtl/math.hxx>
+#include <o3tl/safeint.hxx>
 #include <osl/diagnose.h>
 #include <toolkit/helper/vclunohelper.hxx>
 
@@ -570,7 +571,7 @@ bool DataBrowser::MayMoveLeftColumns() const
     {
         sal_Int32 nColIndex(0);
         if( lcl_SeriesHeaderHasFocus( m_aSeriesHeaders, &nColIndex ))
-            return (static_cast< sal_uInt32 >( nColIndex ) <= (m_aSeriesHeaders.size() - 1)) && (static_cast< sal_uInt32 >( nColIndex ) != 0);
+            return (o3tl::make_unsigned( nColIndex ) <= (m_aSeriesHeaders.size() - 1)) && (static_cast< sal_uInt32 >( nColIndex ) != 0);
     }
 
     sal_Int32 nColIdx = lcl_getColumnInDataOrHeader( GetCurColumnId(), m_aSeriesHeaders );
@@ -587,7 +588,7 @@ bool DataBrowser::MayMoveRightColumns() const
     {
         sal_Int32 nColIndex(0);
         if( lcl_SeriesHeaderHasFocus( m_aSeriesHeaders, &nColIndex ))
-            return (static_cast< sal_uInt32 >( nColIndex ) < (m_aSeriesHeaders.size() - 1));
+            return (o3tl::make_unsigned( nColIndex ) < (m_aSeriesHeaders.size() - 1));
     }
 
     sal_Int32 nColIdx = lcl_getColumnInDataOrHeader( GetCurColumnId(), m_aSeriesHeaders );
diff --git a/chart2/source/controller/dialogs/DataBrowserModel.cxx b/chart2/source/controller/dialogs/DataBrowserModel.cxx
index 272693f1d7c6..e353899eb81e 100644
--- a/chart2/source/controller/dialogs/DataBrowserModel.cxx
+++ b/chart2/source/controller/dialogs/DataBrowserModel.cxx
@@ -41,6 +41,7 @@
 #include <com/sun/star/chart2/data/XNumericalDataSequence.hpp>
 #include <com/sun/star/chart2/data/XTextualDataSequence.hpp>
 #include <com/sun/star/util/XModifiable.hpp>
+#include <o3tl/safeint.hxx>
 #include <tools/diagnose_ex.h>
 #include <comphelper/property.hxx>
 
@@ -291,7 +292,7 @@ void DataBrowserModel::insertDataSeries( sal_Int32 nAfterColumnIndex )
     Reference<chart2::XDiagram> xDiagram = ChartModelHelper::findDiagram(m_xChartDocument);
     Reference<chart2::XChartType> xChartType;
     Reference<chart2::XDataSeries> xSeries;
-    if (static_cast<size_t>(nAfterColumnIndex) < m_aColumns.size())
+    if (o3tl::make_unsigned(nAfterColumnIndex) < m_aColumns.size())
         // Get the data series at specific column position (if available).
         xSeries.set( m_aColumns[nAfterColumnIndex].m_xDataSeries );
 
@@ -436,7 +437,7 @@ void DataBrowserModel::removeComplexCategoryLevel( sal_Int32 nAtColumnIndex )
 void DataBrowserModel::removeDataSeriesOrComplexCategoryLevel( sal_Int32 nAtColumnIndex )
 {
     OSL_ASSERT(m_apDialogModel);
-    if (nAtColumnIndex < 0 || static_cast<size_t>(nAtColumnIndex) >= m_aColumns.size())
+    if (nAtColumnIndex < 0 || o3tl::make_unsigned(nAtColumnIndex) >= m_aColumns.size())
         // Out of bound.
         return;
 
@@ -505,7 +506,7 @@ void DataBrowserModel::removeDataSeriesOrComplexCategoryLevel( sal_Int32 nAtColu
 void DataBrowserModel::swapDataSeries( sal_Int32 nFirstColumnIndex )
 {
     OSL_ASSERT(m_apDialogModel);
-    if( static_cast< tDataColumnVector::size_type >( nFirstColumnIndex ) < m_aColumns.size() - 1 )
+    if( o3tl::make_unsigned( nFirstColumnIndex ) < m_aColumns.size() - 1 )
     {
         Reference< chart2::XDataSeries > xSeries( m_aColumns[nFirstColumnIndex].m_xDataSeries );
         if( xSeries.is())
@@ -728,7 +729,7 @@ sal_Int32 DataBrowserModel::getMaxRowCount() const
 OUString DataBrowserModel::getRoleOfColumn( sal_Int32 nColumnIndex ) const
 {
     if( nColumnIndex != -1 &&
-        static_cast< sal_uInt32 >( nColumnIndex ) < m_aColumns.size())
+        o3tl::make_unsigned( nColumnIndex ) < m_aColumns.size())
         return m_aColumns[ nColumnIndex ].m_aUIRoleName;
     return OUString();
 }
@@ -738,7 +739,7 @@ bool DataBrowserModel::isCategoriesColumn( sal_Int32 nColumnIndex ) const
     if (nColumnIndex < 0)
         return false;
 
-    if (static_cast<size_t>(nColumnIndex) >= m_aColumns.size())
+    if (o3tl::make_unsigned(nColumnIndex) >= m_aColumns.size())
         return false;
 
     // A column is a category when it doesn't have an associated data series.
diff --git a/chart2/source/controller/drawinglayer/ViewElementListProvider.cxx b/chart2/source/controller/drawinglayer/ViewElementListProvider.cxx
index 7113eba8857b..9259ce921819 100644
--- a/chart2/source/controller/drawinglayer/ViewElementListProvider.cxx
+++ b/chart2/source/controller/drawinglayer/ViewElementListProvider.cxx
@@ -24,7 +24,7 @@
 #include <DrawViewWrapper.hxx>
 
 #include <com/sun/star/drawing/Direction3D.hpp>
-
+#include <o3tl/safeint.hxx>
 #include <svx/xtable.hxx>
 #include <svl/itempool.hxx>
 #include <svtools/ctrltool.hxx>
@@ -141,7 +141,7 @@ Graphic ViewElementListProvider::GetSymbolGraphic( sal_Int32 nStandardSymbol, co
         return Graphic();
     if(nStandardSymbol<0)
         nStandardSymbol*=-1;
-    if( static_cast<size_t>(nStandardSymbol) >= pSymbolList->GetObjCount() )
+    if( o3tl::make_unsigned(nStandardSymbol) >= pSymbolList->GetObjCount() )
         nStandardSymbol %= pSymbolList->GetObjCount();
     SdrObject* pObj = pSymbolList->GetObj(nStandardSymbol);
 
diff --git a/chart2/source/controller/main/ElementSelector.cxx b/chart2/source/controller/main/ElementSelector.cxx
index 8ff8f2039abe..4cc61cc1d1b0 100644
--- a/chart2/source/controller/main/ElementSelector.cxx
+++ b/chart2/source/controller/main/ElementSelector.cxx
@@ -27,6 +27,7 @@
 #include <ObjectIdentifier.hxx>
 
 #include <cppuhelper/supportsservice.hxx>
+#include <o3tl/safeint.hxx>
 #include <toolkit/helper/vclunohelper.hxx>
 #include <vcl/svapp.hxx>
 
@@ -182,7 +183,7 @@ void SelectorListBox::Select()
     if ( !IsTravelSelect() )
     {
         const sal_Int32 nPos = GetSelectedEntryPos();
-        if( static_cast<size_t>(nPos) < m_aEntries.size() )
+        if( o3tl::make_unsigned(nPos) < m_aEntries.size() )
         {
             ObjectIdentifier aOID = m_aEntries[nPos].OID;
             Reference< view::XSelectionSupplier > xSelectionSupplier( m_xChartController.get(), uno::UNO_QUERY );
diff --git a/chart2/source/model/main/BaseCoordinateSystem.cxx b/chart2/source/model/main/BaseCoordinateSystem.cxx
index a343cc614a91..7075f2ed6f26 100644
--- a/chart2/source/model/main/BaseCoordinateSystem.cxx
+++ b/chart2/source/model/main/BaseCoordinateSystem.cxx
@@ -26,6 +26,7 @@
 #include <com/sun/star/chart2/AxisType.hpp>
 #include <com/sun/star/container/NoSuchElementException.hpp>
 #include <com/sun/star/lang/IndexOutOfBoundsException.hpp>
+#include <o3tl/safeint.hxx>
 #include <tools/diagnose_ex.h>
 
 #include <algorithm>
@@ -196,7 +197,7 @@ void SAL_CALL BaseCoordinateSystem::setAxisByDimension(
     if( nIndex < 0 )
         throw lang::IndexOutOfBoundsException();
 
-    if( m_aAllAxis[ nDimensionIndex ].size() < static_cast< tAxisVecVecType::size_type >( nIndex+1 ))
+    if( m_aAllAxis[ nDimensionIndex ].size() < o3tl::make_unsigned( nIndex+1 ))
     {
         m_aAllAxis[ nDimensionIndex ].resize( nIndex+1 );
         m_aAllAxis[ nDimensionIndex ][nIndex] = nullptr;
diff --git a/chart2/source/tools/ExplicitCategoriesProvider.cxx b/chart2/source/tools/ExplicitCategoriesProvider.cxx
index 46036e5b9d3f..845b55afda04 100644
--- a/chart2/source/tools/ExplicitCategoriesProvider.cxx
+++ b/chart2/source/tools/ExplicitCategoriesProvider.cxx
@@ -28,6 +28,7 @@
 #include <unonames.hxx>
 
 #include <com/sun/star/chart2/AxisType.hpp>
+#include <o3tl/safeint.hxx>
 #include <tools/diagnose_ex.h>
 
 namespace chart
@@ -360,7 +361,7 @@ static Sequence< OUString > lcl_getExplicitSimpleCategories(
             OUStringBuffer aText;
             for (auto const& complexCatPerIndex : aComplexCatsPerIndex)
             {
-                if ( static_cast<size_t>(nN) < complexCatPerIndex.size() )
+                if ( o3tl::make_unsigned(nN) < complexCatPerIndex.size() )
                 {
                     OUString aAddText = complexCatPerIndex[nN].Text;
                     if( !aAddText.isEmpty() )
diff --git a/chart2/source/view/axes/Tickmarks_Equidistant.hxx b/chart2/source/view/axes/Tickmarks_Equidistant.hxx
index 4c89d4b44251..55263c05ba28 100644
--- a/chart2/source/view/axes/Tickmarks_Equidistant.hxx
+++ b/chart2/source/view/axes/Tickmarks_Equidistant.hxx
@@ -22,6 +22,8 @@
 #include "Tickmarks.hxx"
 #include <memory>
 
+#include <o3tl/safeint.hxx>
+
 namespace chart
 {
 
@@ -58,7 +60,7 @@ private: //methods
                         return (*m_pSimpleTicks)[nDepth][nIndex];
                     else
                     {
-                        if ((*m_pInfoTicks)[nDepth].size() <= size_t(nIndex))
+                        if ((*m_pInfoTicks)[nDepth].size() <= o3tl::make_unsigned(nIndex))
                             return std::numeric_limits<double>::max();
                         return (((*m_pInfoTicks)[nDepth])[nIndex]).fScaledTickValue;
                     }