Date: prev next · Thread: first prev next last
2013 Archives by date, by thread · List index


On Monday 21 of January 2013, Stephan Bergmann wrote:
On 01/18/2013 05:20 PM, Luboš Luňák (via Code Review) wrote:
     https://gerrit.libreoffice.org/1625

I must admit that I didn't review the later revisions of that patch
while in progress at gerrit,

 That's actually from me afterwards, when fixing a problem on 32bit (or was 
that 64bit).

but now the lines 

assert( ll <= SAL_MAX_INT64 ); // valueOfInt64 may not be able to handle
the highest bit

in the various number() function definitions make me wonder.  That
implies that clients of those functions need to ensure to call them with
small-enough arguments.  Is that what we want?

 I think "small-enough" gives the wrong idea, it should be rather, 
say, "not-insanely-large" arguments or so :). And despite the smiley, I'm 
actually serious. This can trigger only when needing the topmost unsigned bit 
of 64bit value, which is a 20 digit decimal number, and more importantly 
those are values at the very limit of the supported integer range - there's 
nothing larger than long long.

 So even if there will be a legitimate case when unsigned long long will be 
needed for the extra bit (which is pretty unlikely already), a lot of care 
will be needed to handle it correctly (e.g. involving anything signed will be 
potential trouble). That'll make this problem just a little bit more of 
trouble.

While the gotcha of printing a large unsigned value as a negative value
thanks to calling rtl_ustr_valueOfInt64 internally can be a problem in
some call sites, others might be fine with producing just some sort of
informative value and won't mind generating negative output.  If we
would want to force the latter into using explicit casts to sal_Int64
(in case they don't do already anyway), wouldn't it be better to make
the relevant large unsigned overloads "= delete"?

 That would mean blocking out all the values of the type, not just the 
problematic ones. Given that a number of system typedefs are internally 
unsigned integers despite all the signed vs unsigned trouble this causes, 
usage of the type is much more likely than usage of a problematic value of 
the type, so IMO it makes more sense to cater to realistic usage rathen than 
handle more problematic but next to impossible scenarios.

-- 
 Lubos Lunak
 l.lunak@suse.cz

Context


Privacy Policy | Impressum (Legal Info) | Copyright information: Unless otherwise specified, all text and images on this website are licensed under the Creative Commons Attribution-Share Alike 3.0 License. This does not include the source code of LibreOffice, which is licensed under the Mozilla Public License (MPLv2). "LibreOffice" and "The Document Foundation" are registered trademarks of their corresponding registered owners or are in actual use as trademarks in one or more countries. Their respective logos and icons are also subject to international copyright laws. Use thereof is explained in our trademark policy.