Re: o3tl::make_unsigned

On Thursday 30 of January 2020, Stephan Bergmann wrote:
> On 30/01/2020 12:56, Luboš Luňák wrote:
> >   This contradicts your original mail, where you stated that what is used
> > is "if (sal_uInt32(e1) < e2) ...    // (B)" (i.e. without the <0 check).
> > And
>
> Maybe it is unclear, but my original mail was talking about a "correct"
> comparison
>
>    e1 < e2
>
> (i.e., where it is known that e1 >= 0, but where compilers might
> nevertheless emit a signed-vs.-unsigned warning).

 Then make_unsigned() covers a smaller problem area than make_signed(), which 
covers all cases except for the exceptional totally huge value cases. I.e. 
with make_signed() you practically do not need to care what the values in "e1 
< e2" are. Why make API that requires caring about the value when there can 
be API that makes it practically irrelevant?

> >   It doesn't pretend any more than make_unsigned() does. It's just that
> > it's harder to accidentally break its requirements in practice.
>
> ...while it's probably easier to write code that is guaranteed to comply
> with the make_unsigned preconditions.

 No? I'd have to try really hard to write realistic code that uses highest bit 
set in a large unsigned non-bitfield variable.

> (Also, I have no idea what the usage patterns for actual types S1 and U2
> are.  For usages with small U2,

 Small unsigned types are promoted at least to int. If needed, make_signed() 
can promote them to long long if needed (probably not). And if they are to be 
compared using <, then their usage pattern will be that U2 is a count of 
something.

> or e2 obtained from tainted sources, 
> casting to S2 may well be an issue wrt preconditions.)

 Such as?

> >   But people will not consistently use (*), because it's more
> > error-prone, longer to type and harder to read (yeah, we're all lazy).
> > Using just "e1 < S1
>
> I'm not sure about that.  As I wrote, I saw lots of occurrences of (*)
> across the LO code base.

 And how many occurences that do not do (*) have you seen? (I have no idea how 
to reasonably search for those).

> (And, again, people writing broken code is 
> something that o3tl::make_unsigned cannot and will not address.)

 And, yet, o3tl::make_signed would address that, for all cases that matter.

> > (e2)" can assert in S1 and then the only disadvantage there I can see is
> > if somebody actually somehow manages to trigger the assert =>
> > make_signed() is the better API.
>
> I don't understand the above sentence; what do you mean with "can assert
> in S1"?

 e2 > std::numeric_limits<S1>::max() || e1 < S1(e2)

 can be rewritten as

 S1 make_signed(U2 v) { assert( v <= std::numeric_limits<S1>::max()); return 
v; }

 and then

 e1 < make_signed(e2)

 pretty much always does the job. And we'll probably hit the 25th LO 
anniversary sooner than this assert.

-- 
 Luboš Luňák
 l.lunak@collabora.com