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
_______________________________________________
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice
