Fix big(typemin(T))

[martinholters]

master:

julia> big(typemin(Int8))
-18446744073709551488

julia> big(typemin(Int16))
-18446744073709518848

this PR:

julia> big(typemin(Int8))
-128

julia> big(typemin(Int16))
-32768

(Bug was introduced by #29165, i.e. after 1.0.)

[martinholters]

Obviously, this may still fail for other integer types than the above if they use two's complement (or some other representation that does not allow every negative value to be negated without overflow). I'm not sure what to do in the general case, though. At least check that x > 0 after negation and throw an error otherwise to prevent silently wrong results?

[rfourquet]

I think we need to have an abstract type between Signed and those builtin types to represent this class of types, e.g. SignedTwoComplement or something (or a trait indicating that Unsigned can be applied to the number). For now, what about a mix of your solution and your idea to check that x > 0:

s == -1 && (x = -x)
if x < 0
    x = s == -1 ? Unsigned(~x) + one(x) : Unsigned(x)
end

This may throw at the Unsigned call for number which don't support that, but only if the basic x = -x already fails.
Would there be a type-instability performance problem due to having x <: Unsigned based on a dynamic condition?

[martinholters]

Yes, that would introduce type-instability, although the performance-penalty may be acceptably low with the Union optimizations. However, just the fact that x < 0 && -x < 0 does not necessarily mean that x is in two complement (although it is very likely, of course.)

I've also considered introducing an unsigned_abs that returns the absolute value as an Unsigned type (or rather, a type which can definitely hold abs(x) and may be unsigned) which could then be implemented for new types as needed. Introducing SignedTwoComplement sounds more attractive to me, but adding a new abstract type in the Numberhierarchy is, of course, a change that should be well thought through.

That said, the present code is simple and a strict improvement 😄

[martinholters]

Ah, but that should be unsigned rather than Unsigned because the latter unnecessarily checks that its argument is not negative which we already know here. Will update locally and push once we agree on the general path forward here.

[martinholters]

What if we do the negation on the individual limbs which we know are in two complement? Would require having a carry to be carried across the loop, but that does not sound too bad. Barring better ideas, I'll give it a shot tomorrow.

[rfourquet]

But doens't that assume anyway two's complement? the previous code (before the bug) was doing something like that, taking the bit pattern of x < 0 and at the end returning -b-1 where b was the constructed BigInt. So I actually think we can assume here two's complement without breaking code.
One of my motivation doing this change was to accelerate converting bigger primitive types, like Int512, so it would be great if the fix works for non-base types!
Don't you think that if x is transformed to Unsigned in the case x < 0 && -x < 0, then the type-instability possible performance hit would occur only for this rare corner case? This seems simpler than implementing the "carry carried across the loop".

[oscardssmith]

What's wrong with the new behavior?

[martinholters]

"Before" = master
"After" = this PR
(Clarified in OP)
Does that answer your question, or is the question indeed what's wrong with the behavior of master?

[oscardssmith]

Yep, makes sense. I got the order backwards.

[simonbyrne]

Is this actually that useful of a test? If the comparison uses promotion, then it won't work correctly.

[martinholters]

True. Currently, comparison does not promote (that's how I stumbled upon this), but if it does in the future, this test will become useless. I'll change to use explicit constants.

[martinholters]

Ok, having slept this over, KISS seems best here.

I'm not even sure we can truely support general, non-two complement integer representations. Apart from the negation I deal with here, we also use >>>, which is defined as a bit shift, which may not make any sense for arbitrary integer representations. If we rewrite that as ÷ 2^n, we assume the division to be well-behaved (for all inputs), which is likely, but in no way guaranteed. I think we would need a fused (x ÷ 2^n) % Toperation, which (for suitable type T) does the right thing and can be implemented by custom integer types. I doubt it's worth it and definitely beyond the scope of this PR which is intended to be a small bugfix.

[rfourquet]

Thanks for this beautiful KISS solution!