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

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