digits!: check for typemax only when applicable

[rfourquet]

No description provided.

[simonbyrne]

why not applicable(typemax, T) && ...?

[rfourquet]

Because then the second branch of the && must be parenthesized (one more character, you know!)

[rfourquet]

Would it be clearer if I change the test to:

applicable(typemax, T) &&
        base - 1 > typemax(T) && throw(ArgumentError("type $T too small for base $base"))

?

[simonbyrne]

I'm not sure we really want to call applicable here: based on the @code_llvm it doesn't seem to be evaluated only at compile time. Unless anyone can think of something better, it might be better to just have T == BigInt || base-1 <= typemax(T) || ...

[rfourquet]

Oh, I didn't expect that to be a performance problem, but still it seems to be reasonable (of the order of 30 percent penalty for small numbers). I just tried with method_exists instead, but this is far worse. I don't like the T == BigInt solution, as it doesn't handle number types defined outside of Base.

[nalimilan]

See #16422. What do you mean by "far worse"?

[rfourquet]

Ah thanks, I had seen that one but totally forgot about it. As @timholy seemed to want it merged soon, I will update this PR using method_exists, and hope for the best. For now, this is one order of magnitude slower (for the digits of small numbers).

[TotalVerb]

I think that issue has a high chance of getting closed without merging, as @timholy noted in reply to Jameson's comments on its dangerous implementation.

[rfourquet]

OK so let's not hold our breath. I just tried a new version where I replace applicable by hastypemax defined as follows:

hastypemax{T<:Base.BitInteger}(::Type{T}) = true
hastypemax{T<:BigInt}(::Type{T}) = false
hastypemax(::Type{T}) = applicable(typemax, T)

The perfs seem to be similar to the base version, maybe few percents behind (hastypemax may be not removed at compilation time?). But it's a bit ugly, I'd rather wait for applicable (or method_exists) to be improved, and pay 30% penalty in the meantime. But if someone thinks it's important to preserve every bit of efficiency in digits!, I will update with this hastypemax function.

[rfourquet]

Related to #7501.

[rfourquet]

I updated with the hastypemax solution (outlined above #16844 (comment)), which is the most conservative w.r.t. performances. This is a rather trivial fix, I will merge soon if no objection.

[StefanKarpinski]

This is a bugfix, so it should be backportable; however, it does introduce a new internal trait. How do you feel about potentially backporting this, @tkelman?

[tkelman]

None of the existing traits would cover this?

[Sacha0]

superficially lgtm (i.e. modulo @tkelman's hastypemaxquestion)! :)

[rfourquet]

Rebased, to trigger an up-to-date build before merge :)
Also, I updated ::Type{<:BitInteger} to ::Union{Type{Int8}, ...}, similarly as was done in #22403.
@tkelman: I am not aware of an already existing trait covering this, do you have a hint were to look at?

[tkelman]

Some of them are starting to get added in base/traits.jl - specifically here, would the existing uses of TypeArithmetic(::Type{<:Integer}) = ArithmeticOverflows() be more or less correct if BigInt was changed to a different value?

[rfourquet]

would the existing uses of TypeArithmetic(::Type{<:Integer}) = ArithmeticOverflows() be more or less correct if BigInt was changed to a different value?

I think yes, partially. But as you say, this would require to change BigIngt like e.g. TypeArithmetic(::Type{BigInt}) = ArithmeticUnbounded(), which in turns needs to be taken into account in other places, like "range.jl". Also, this wouldn't directly handle the case of custom (unbounded) integer types, for which I call applicable here, unless the author specializes TypeArithmetic, which means the bug would not be totally fixed; so I'm a bit reluctant.

[rfourquet]

Will merge within one or two days, if no objection.

[nalimilan]

This looks like another case where a trait would not really be needed if applicable/method_exists was resolved statically (#16422).