summaryrefslogtreecommitdiff
path: root/emfio/source
diff options
context:
space:
mode:
authorStephan Bergmann <sbergman@redhat.com>2020-01-27 09:30:39 +0100
committerStephan Bergmann <sbergman@redhat.com>2020-01-28 07:42:15 +0100
commitaef7feb3e695ecf6d411f0777196dcc4281e201a (patch)
tree6adff7e08e6431ff87c575d026e330badb9a6cd3 /emfio/source
parent65f007c629e5a7b2710e21e3f26164b433576e27 (diff)
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>
Diffstat (limited to 'emfio/source')
-rw-r--r--emfio/source/reader/mtftools.cxx6
-rw-r--r--emfio/source/reader/wmfreader.cxx3
2 files changed, 5 insertions, 4 deletions
diff --git a/emfio/source/reader/mtftools.cxx b/emfio/source/reader/mtftools.cxx
index 461109067f66..15ede2bf6d40 100644
--- a/emfio/source/reader/mtftools.cxx
+++ b/emfio/source/reader/mtftools.cxx
@@ -621,7 +621,7 @@ namespace emfio
GDIObj *pGDIObj = nullptr;
- if ( static_cast<sal_uInt32>(nIndex) < mvGDIObj.size() )
+ if ( o3tl::make_unsigned(nIndex) < mvGDIObj.size() )
pGDIObj = mvGDIObj[ nIndex ].get();
if ( pGDIObj )
@@ -764,7 +764,7 @@ namespace emfio
}
}
}
- if ( static_cast<sal_uInt32>(nIndex) >= mvGDIObj.size() )
+ if ( o3tl::make_unsigned(nIndex) >= mvGDIObj.size() )
ImplResizeObjectArry( nIndex + 16 );
mvGDIObj[ nIndex ] = std::move(pObject);
@@ -780,7 +780,7 @@ namespace emfio
{
if ( ( nIndex & ENHMETA_STOCK_OBJECT ) == 0 )
{
- if ( static_cast<sal_uInt32>(nIndex) < mvGDIObj.size() )
+ if ( o3tl::make_unsigned(nIndex) < mvGDIObj.size() )
{
mvGDIObj[ nIndex ].reset();
}
diff --git a/emfio/source/reader/wmfreader.cxx b/emfio/source/reader/wmfreader.cxx
index a070d8cfbea3..7249b61017ff 100644
--- a/emfio/source/reader/wmfreader.cxx
+++ b/emfio/source/reader/wmfreader.cxx
@@ -22,6 +22,7 @@
#include <memory>
#include <o3tl/optional.hxx>
+#include <o3tl/safeint.hxx>
#include <rtl/crc.h>
#include <rtl/tencinfo.h>
#include <sal/log.hxx>
@@ -590,7 +591,7 @@ namespace emfio
auto nMaxStreamPos = nRecordPos + nRecSize;
auto nRemainingSize = std::min(mpInputStream->remainingSize(), nMaxStreamPos - mpInputStream->Tell());
- if (nRemainingSize < static_cast<sal_uInt32>(nOriginalBlockLen))
+ if (nRemainingSize < o3tl::make_unsigned(nOriginalBlockLen))
{
SAL_WARN("vcl.wmf", "exttextout record claimed more data than the stream can provide");
nOriginalTextLen = nOriginalBlockLen = nRemainingSize;