[Libreoffice] signed/unsigned comparison (was: [PATCH] Replace SvULongs with vector and code clean up part 1)

[Stephan Bergmann]

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.

-Stephan