[Libreoffice] signed/unsigned comparison (was: [PATCH] Replace SvULongs with vector and code clean up part 1)
stephan.bergmann.secondary at googlemail.com
Tue Aug 9 00:05:49 PDT 2011
On Aug 6, 2011, at 3:32 PM, Maciej Rumianowski wrote:
> I was working on SvULongs in libs-core and I decided to split my work in
> several Patches.
> With this patch comes some questions:
>> @@ -228,9 +229,9 @@ private:
>> String aValStr;
>> double nValNum;
>> sal_Bool bUndoAddList;
>> - SvULongs aAddList;
>> - SvULongs aDelList;
>> - SvULongs aCurEntryList;
>> + std::vector<sal_uInt32> aAddList;
>> + std::vector<sal_uInt32> aDelList;
>> + std::vector<sal_uInt32> aCurEntryList;
>> @@ -1325,7 +1288,7 @@ String SvxNumberFormatShell::GetComment4Entry(short nEntry)
>> if(nEntry < 0)
>> return String();
>> - if(nEntry<aCurEntryList.Count())
>> + if(nEntry < (short)aCurEntryList.size())
> 5. Should short type be replaced with sal_Int16 or more appropriate type?
Some C++ compilers warn about comparisons like (x < y) where x is an rvalue of a signed integral type and y is an rvalue of an unsigned integral type. And, in general, they are right in doing so: Assume that x is an rvalue -1 of type int and y is some rvalue of type unsigned int. Usual arithmetic conversion causes the int -1 to be converted to unsigned int. Undefined behavior, hard-disk erased. Oops.
However, if the programmer can prove that x is always non-negative in the above comparison, there is usually an easy solution to that problem:
First, assert the programmer's knowledge that x is indeed non-negative. In the original code, that would be
OSL_ASSERT(nEntry >= 0);
(Which might or might not be considered overkill in this specific case, given the if(nEntry < 0) branch just above the code in question.)
Next, cast x (of signed integral type T) to an unsigned integral type that is known to be able to represent all the non-negative values of T. (In general, with current C++ implementations, that can be tricky if all you know about T is that it is some typedef.) But in the original code T is known to be short (and whether that is a good choice is an entirely different question), so that code could be changed to
if (sal::static_int_cast< unsigned short >(nEntry) < aCurEntryList.size())
Note the use of sal::static_int_cast instead of static_cast---the former was introduced to make it simpler to find all places that need to be adapted should the type of nEntry ever be changed---and is declared and documented in sal/types.h. (And never use a C-style cast in C++ code.) Note also that it would also work to cast to a larger type than unsigned short, like unsigned int or unsigned long int.
More information about the LibreOffice