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>

9 files changed, 32 insertions, 21 deletions

diff --git a/connectivity/source/commontools/dbtools.cxx b/connectivity/source/commontools/dbtools.cxx
index 629c5cd074e2..9d4dcd18106d 100644
--- a/connectivity/source/commontools/dbtools.cxx
+++ b/connectivity/source/commontools/dbtools.cxx
@@ -75,6 +75,7 @@
 #include <connectivity/dbtools.hxx>
 #include <connectivity/statementcomposer.hxx>
 #include <o3tl/any.hxx>
+#include <o3tl/safeint.hxx>
 #include <osl/diagnose.h>
 #include <rtl/ustrbuf.hxx>
 #include <sal/log.hxx>
@@ -1659,7 +1660,7 @@ namespace
         {
             if ( m_aSet.empty() )
                 return m_xSource->getByIndex(Index);
-            if ( m_aSet.size() < static_cast<size_t>(Index) )
+            if ( Index < 0 || m_aSet.size() < o3tl::make_unsigned(Index) )
                 throw IndexOutOfBoundsException();
 
             std::vector<bool, std::allocator<bool> >::const_iterator aIter = m_aSet.begin();
diff --git a/connectivity/source/commontools/parameters.cxx b/connectivity/source/commontools/parameters.cxx
index f7a2d73e109b..6397ea6f783d 100644
--- a/connectivity/source/commontools/parameters.cxx
+++ b/connectivity/source/commontools/parameters.cxx
@@ -36,6 +36,7 @@
 #include <tools/diagnose_ex.h>
 
 #include <connectivity/ParameterCont.hxx>
+#include <o3tl/safeint.hxx>
 #include <rtl/ustrbuf.hxx>
 #include <sal/log.hxx>
 
@@ -468,7 +469,7 @@ namespace dbtools
             size_t nAlreadyVisited = 0;
             for (auto & aIndex : aParam.second.aInnerIndexes)
             {
-                if ( ( m_aParametersVisited.size() > static_cast<size_t>(aIndex) ) && m_aParametersVisited[ aIndex ] )
+                if ( ( m_aParametersVisited.size() > o3tl::make_unsigned(aIndex) ) && m_aParametersVisited[ aIndex ] )
                 {   // exclude this index
                     aIndex = -1;
                     ++nAlreadyVisited;
@@ -949,7 +950,7 @@ namespace dbtools
 
     void ParameterManager::externalParameterVisited( sal_Int32 _nIndex )
     {
-        if ( m_aParametersVisited.size() < static_cast<size_t>(_nIndex) )
+        if ( m_aParametersVisited.size() < o3tl::make_unsigned(_nIndex) )
         {
             m_aParametersVisited.reserve( _nIndex );
             for ( sal_Int32 i = m_aParametersVisited.size(); i < _nIndex; ++i )
diff --git a/connectivity/source/drivers/dbase/DTable.cxx b/connectivity/source/drivers/dbase/DTable.cxx
index 06d0fdf7d576..5fc64ab84625 100644
--- a/connectivity/source/drivers/dbase/DTable.cxx
+++ b/connectivity/source/drivers/dbase/DTable.cxx
@@ -23,6 +23,7 @@
 #include <com/sun/star/sdbc/DataType.hpp>
 #include <com/sun/star/ucb/XContentAccess.hpp>
 #include <com/sun/star/sdbc/XRow.hpp>
+#include <o3tl/safeint.hxx>
 #include <svl/converter.hxx>
 #include <dbase/DConnection.hxx>
 #include <dbase/DColumns.hxx>
@@ -848,7 +849,7 @@ bool ODbaseTable::fetchRow(OValueRefRow& _rRow, const OSQLColumns & _rCols, bool
         else if ( DataType::INTEGER == nType )
         {
             sal_Int32 nValue = 0;
-            if (static_cast<size_t>(nLen) > sizeof(nValue))
+            if (o3tl::make_unsigned(nLen) > sizeof(nValue))
                 return false;
             memcpy(&nValue, pData, nLen);
             *(_rRow->get())[i] = nValue;
@@ -859,7 +860,7 @@ bool ODbaseTable::fetchRow(OValueRefRow& _rRow, const OSQLColumns & _rCols, bool
             if (getBOOL((*aIter)->getPropertyValue(OMetaConnection::getPropMap().getNameByIndex(PROPERTY_ID_ISCURRENCY)))) // Currency is treated separately
             {
                 sal_Int64 nValue = 0;
-                if (static_cast<size_t>(nLen) > sizeof(nValue))
+                if (o3tl::make_unsigned(nLen) > sizeof(nValue))
                     return false;
                 memcpy(&nValue, pData, nLen);
 
@@ -870,7 +871,7 @@ bool ODbaseTable::fetchRow(OValueRefRow& _rRow, const OSQLColumns & _rCols, bool
             }
             else
             {
-                if (static_cast<size_t>(nLen) > sizeof(d))
+                if (o3tl::make_unsigned(nLen) > sizeof(d))
                     return false;
                 memcpy(&d, pData, nLen);
             }
@@ -1697,11 +1698,11 @@ bool ODbaseTable::UpdateBuffer(OValueRefVector& rRow, const OValueRefRow& pOrgRo
     {
         // Lengths for each data type:
         assert(i >= 0);
-        OSL_ENSURE(sal_uInt32(i) < m_aPrecisions.size(),"Illegal index!");
+        OSL_ENSURE(o3tl::make_unsigned(i) < m_aPrecisions.size(),"Illegal index!");
         sal_Int32 nLen = 0;
         sal_Int32 nType = 0;
         sal_Int32 nScale = 0;
-        if ( sal_uInt32(i) < m_aPrecisions.size() )
+        if ( o3tl::make_unsigned(i) < m_aPrecisions.size() )
         {
             nLen    = m_aPrecisions[i];
             nType   = m_aTypes[i];
@@ -1832,7 +1833,7 @@ bool ODbaseTable::UpdateBuffer(OValueRefVector& rRow, const OValueRefRow& pOrgRo
                 case DataType::INTEGER:
                     {
                         sal_Int32 nValue = thisColVal;
-                        if (static_cast<size_t>(nLen) > sizeof(nValue))
+                        if (o3tl::make_unsigned(nLen) > sizeof(nValue))
                             return false;
                         memcpy(pData,&nValue,nLen);
                     }
@@ -1849,13 +1850,13 @@ bool ODbaseTable::UpdateBuffer(OValueRefVector& rRow, const OValueRefRow& pOrgRo
                                 nValue = static_cast<sal_Int64>(d * pow(10.0,static_cast<int>(m_aScales[i])));
                             else
                                 nValue = static_cast<sal_Int64>(d);
-                            if (static_cast<size_t>(nLen) > sizeof(nValue))
+                            if (o3tl::make_unsigned(nLen) > sizeof(nValue))
                                 return false;
                             memcpy(pData,&nValue,nLen);
                         } // if (getBOOL(xCol->getPropertyValue(OMetaConnection::getPropMap().getNameByIndex(PROPERTY_ID_ISCURRENCY)))) // Currency is treated separately
                         else
                         {
-                            if (static_cast<size_t>(nLen) > sizeof(d))
+                            if (o3tl::make_unsigned(nLen) > sizeof(d))
                                 return false;
                             memcpy(pData,&d,nLen);
                         }
diff --git a/connectivity/source/drivers/dbase/dindexnode.cxx b/connectivity/source/drivers/dbase/dindexnode.cxx
index 43db753bdaf9..4b1c24057020 100644
--- a/connectivity/source/drivers/dbase/dindexnode.cxx
+++ b/connectivity/source/drivers/dbase/dindexnode.cxx
@@ -19,6 +19,7 @@
 
 #include <dbase/dindexnode.hxx>
 #include <dbase/DIndex.hxx>
+#include <o3tl/safeint.hxx>
 #include <tools/debug.hxx>
 #include <tools/stream.hxx>
 #include <sal/log.hxx>
@@ -245,7 +246,7 @@ bool ONDXPage::Insert(ONDXNode& rNode, sal_uInt32 nRowsLeft)
         // How many nodes are being inserted?
         // Enough, then we can fill the page to the brim
         ONDXNode aInnerNode;
-        if (!IsLeaf() || nRowsLeft < static_cast<sal_uInt32>(rIndex.GetMaxNodes() / 2))
+        if (!IsLeaf() || nRowsLeft < o3tl::make_unsigned(rIndex.GetMaxNodes() / 2))
             aInnerNode = Split(*aNewPage);
         else
         {
diff --git a/connectivity/source/drivers/file/FStatement.cxx b/connectivity/source/drivers/file/FStatement.cxx
index 8e25a34dac88..a3fd14970b22 100644
--- a/connectivity/source/drivers/file/FStatement.cxx
+++ b/connectivity/source/drivers/file/FStatement.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 <osl/diagnose.h>
 #include <file/FStatement.hxx>
 #include <file/FConnection.hxx>
@@ -606,7 +609,7 @@ void OStatement_Base::GetAssignValues()
 
 void OStatement_Base::ParseAssignValues(const std::vector< OUString>& aColumnNameList,OSQLParseNode* pRow_Value_Constructor_Elem, sal_Int32 nIndex)
 {
-    OSL_ENSURE(size_t(nIndex) <= aColumnNameList.size(),"SdbFileCursor::ParseAssignValues: nIndex > aColumnNameList.GetTokenCount()");
+    OSL_ENSURE(o3tl::make_unsigned(nIndex) <= aColumnNameList.size(),"SdbFileCursor::ParseAssignValues: nIndex > aColumnNameList.GetTokenCount()");
     OUString aColumnName(aColumnNameList[nIndex]);
     OSL_ENSURE(aColumnName.getLength() > 0,"OResultSet: Column-Name not found");
     OSL_ENSURE(pRow_Value_Constructor_Elem != nullptr,"OResultSet: pRow_Value_Constructor_Elem must not be NULL!");
diff --git a/connectivity/source/drivers/flat/ETable.cxx b/connectivity/source/drivers/flat/ETable.cxx
index 142d191be8c9..6d9fc203f950 100644
--- a/connectivity/source/drivers/flat/ETable.cxx
+++ b/connectivity/source/drivers/flat/ETable.cxx
@@ -24,6 +24,7 @@
 #include <com/sun/star/ucb/XContentAccess.hpp>
 #include <flat/EConnection.hxx>
 #include <flat/EColumns.hxx>
+#include <o3tl/safeint.hxx>
 #include <rtl/math.hxx>
 #include <sal/log.hxx>
 #include <tools/solar.h>
@@ -728,7 +729,7 @@ bool OFlatTable::seekRow(IResultSetHelper::Movement eCursorPosition, sal_Int32 n
                 if(m_nMaxRowCount != 0 && m_nRowPos > m_nMaxRowCount)
                     return false;
                 ++m_nRowPos;
-                if(m_aRowPosToFilePos.size() > static_cast< vector< TRowPositionInFile >::size_type >(m_nRowPos))
+                if(m_aRowPosToFilePos.size() > o3tl::make_unsigned(m_nRowPos))
                 {
                     m_bNeedToReadLine = true;
                     m_nFilePos  = m_aRowPosToFilePos[m_nRowPos].first;
@@ -768,7 +769,7 @@ bool OFlatTable::seekRow(IResultSetHelper::Movement eCursorPosition, sal_Int32 n
             --m_nRowPos;
             {
                 assert (m_nRowPos >= 0);
-                assert(m_aRowPosToFilePos.size() >= static_cast< vector< TRowPositionInFile >::size_type >(m_nRowPos));
+                assert(m_aRowPosToFilePos.size() >= o3tl::make_unsigned(m_nRowPos));
                 const TRowPositionInFile &aPositions(m_aRowPosToFilePos[m_nRowPos]);
                 m_nFilePos = aPositions.first;
                 nCurPos = aPositions.second;
@@ -819,9 +820,9 @@ bool OFlatTable::seekRow(IResultSetHelper::Movement eCursorPosition, sal_Int32 n
                 }
 
                 assert(m_nRowPos >=0);
-                assert(m_aRowPosToFilePos.size() > static_cast< vector< TRowPositionInFile >::size_type >(m_nRowPos));
+                assert(m_aRowPosToFilePos.size() > o3tl::make_unsigned(m_nRowPos));
                 assert(nOffset >= 0);
-                if(m_aRowPosToFilePos.size() > static_cast< vector< TRowPositionInFile >::size_type >(nOffset))
+                if(m_aRowPosToFilePos.size() > o3tl::make_unsigned(nOffset))
                 {
                     m_nFilePos  = m_aRowPosToFilePos[nOffset].first;
                     nCurPos     = m_aRowPosToFilePos[nOffset].second;
diff --git a/connectivity/source/drivers/mork/MResultSet.cxx b/connectivity/source/drivers/mork/MResultSet.cxx
index 2275472c5a4a..d5cc06559e11 100644
--- a/connectivity/source/drivers/mork/MResultSet.cxx
+++ b/connectivity/source/drivers/mork/MResultSet.cxx
@@ -25,6 +25,7 @@
 #include <connectivity/dbtools.hxx>
 #include <comphelper/types.hxx>
 #include <cppuhelper/typeprovider.hxx>
+#include <o3tl/safeint.hxx>
 #include <sal/log.hxx>
 
 #include <vector>
@@ -1289,7 +1290,7 @@ bool OResultSet::validRow( sal_uInt32 nRow)
     sal_Int32  nNumberOfRecords = m_aQueryHelper.getResultCount();
 
     if (( nRow == 0 ) ||
-        ( nRow > static_cast<sal_uInt32>(nNumberOfRecords)) ){
+        ( nRow > o3tl::make_unsigned(nNumberOfRecords)) ){
         SAL_INFO("connectivity.mork", "validRow(" << nRow << "): return False");
         return false;
     }
diff --git a/connectivity/source/drivers/odbc/OResultSet.cxx b/connectivity/source/drivers/odbc/OResultSet.cxx
index e9345aeae6c9..e17032152433 100644
--- a/connectivity/source/drivers/odbc/OResultSet.cxx
+++ b/connectivity/source/drivers/odbc/OResultSet.cxx
@@ -33,6 +33,7 @@
 #include <comphelper/types.hxx>
 #include <connectivity/dbtools.hxx>
 #include <connectivity/dbexception.hxx>
+#include <o3tl/safeint.hxx>
 #include <sal/log.hxx>
 
 using namespace ::comphelper;
@@ -815,7 +816,7 @@ void SAL_CALL OResultSet::insertRow(  )
 
     SQLLEN nRealLen = 0;
     Sequence<sal_Int8> aBookmark(nMaxBookmarkLen);
-    static_assert(static_cast<size_t>(nMaxBookmarkLen) >= sizeof(SQLLEN), "must be larger");
+    static_assert(o3tl::make_unsigned(nMaxBookmarkLen) >= sizeof(SQLLEN), "must be larger");
 
     SQLRETURN nRet = N3SQLBindCol(m_aStatementHandle,
                                 0,
diff --git a/connectivity/source/drivers/odbc/OTools.cxx b/connectivity/source/drivers/odbc/OTools.cxx
index fb0dc71d716f..4781415de474 100644
--- a/connectivity/source/drivers/odbc/OTools.cxx
+++ b/connectivity/source/drivers/odbc/OTools.cxx
@@ -20,6 +20,7 @@
 #include <odbc/OTools.hxx>
 #include <odbc/OFunctions.hxx>
 #include <com/sun/star/sdbc/DataType.hpp>
+#include <o3tl/safeint.hxx>
 #include <osl/diagnose.h>
 #include <osl/endian.h>
 #include <odbc/OConnection.hxx>
@@ -137,7 +138,7 @@ void OTools::getValue(  OConnection const * _pConnection,
     else
     {
         OSL_ENSURE(static_cast<size_t>(_nSize) == properSize, "connectivity::odbc::OTools::getValue got wrongly sized memory region to write result to");
-        if ( static_cast<size_t>(_nSize) > properSize )
+        if ( o3tl::make_unsigned(_nSize) > properSize )
         {
             SAL_WARN( "connectivity.drivers", "memory region is too big - trying to fudge it");
             memset(_pValue, 0, _nSize);
@@ -147,7 +148,7 @@ void OTools::getValue(  OConnection const * _pConnection,
 #endif
         }
     }
-    OSL_ENSURE(static_cast<size_t>(_nSize) >= properSize, "memory region is too small");
+    OSL_ENSURE(o3tl::make_unsigned(_nSize) >= properSize, "memory region is too small");
     SQLLEN pcbValue = SQL_NULL_DATA;
     OTools::ThrowException(_pConnection,
                             (*reinterpret_cast<T3SQLGetData>(_pConnection->getOdbcFunction(ODBC3SQLFunctionId::GetData)))(_aStatementHandle,