Skip to content

Conversation

Liozou
Copy link
Member

@Liozou Liozou commented Sep 22, 2025

Add gcdx(a::Signed, b::Unsigned) and gcdx(a::Unsigned, b::Signed) methods to fix #58025:

julia> gcdx(UInt16(100), Int8(-101))  # pr
(0x0001, 0xffff, 0xffff)

julia> gcdx(UInt16(100), Int8(-101))  # master, incorrect result
(0x0005, 0xf855, 0x0003)

Also add the equivalent methods for lcm to fix the systematic InexactError when one argument is a negative Signed and the other is any Unsigned:

julia> lcm(UInt16(100), Int8(-101))  # pr
0x2774

julia> lcm(UInt16(100), Int8(-101))  # master, error
ERROR: InexactError: trunc(UInt16, -101)
Stacktrace:
 [1] throw_inexacterror(func::Symbol, to::Type, val::Int8)
   @ Core ./boot.jl:866
 [2] check_sign_bit
   @ ./boot.jl:872 [inlined]
 [3] toUInt16
   @ ./boot.jl:958 [inlined]
 [4] UInt16
   @ ./boot.jl:1011 [inlined]
 [5] convert
   @ ./number.jl:7 [inlined]
 [6] _promote
   @ ./promotion.jl:379 [inlined]
 [7] promote
   @ ./promotion.jl:404 [inlined]
 [8] lcm(a::UInt16, b::Int8)
   @ Base ./intfuncs.jl:152
 [9] top-level scope
   @ REPL[62]:1

Inspired by #59487 (comment). The difference is that the solution proposed in this PR keeps the current correct result type for inputs such as (::Int16, ::UInt8).

@Liozou Liozou added maths Mathematical functions bugfix This change fixes an existing bug labels Sep 22, 2025
@Liozou Liozou force-pushed the gcdxsignedunsigned branch 2 times, most recently from 0569b5f to c44a3f6 Compare September 22, 2025 22:00
@adienes
Copy link
Member

adienes commented Sep 23, 2025

nice! I ended up with something very similar (locally).

I think it would be good if we had a clear policy about which inputs are allowed for all of gcd, gcdx, lcm. something like

if the T = promote_type is signed and one of the arguments is that type's typemin, an error will be thrown. otherwise you get the correct value in T

as it stands, this error may be thrown in several places throughout the gcd functions: in promote, in checked_abs, or in divrem which I find a bit confusing

@Liozou
Copy link
Member Author

Liozou commented Sep 23, 2025

Indeed! With this PR, the behaviour of lcm becomes identical to that of gcd, so it will still throw an InexactError in the specific case of of lcm(typemin(IntX1), UIntX2) where X1 < X2 (you phrased it better, but it's the same condition). The proposed gcdx does not suffer from this limitation: with this PR,

julia> lcm(typemin(Int8), UInt16(256))
ERROR: InexactError: trunc(UInt16, -128)
Stacktrace:
 [1] throw_inexacterror(func::Symbol, to::Type, val::Int8)
   @ Core ./boot.jl:866
 [2] check_sign_bit
   @ ./boot.jl:872 [inlined]
 [3] toUInt16
   @ ./boot.jl:958 [inlined]
 [4] UInt16
   @ ./boot.jl:1011 [inlined]
 [5] convert
   @ ./number.jl:7 [inlined]
 [6] _promote
   @ ./promotion.jl:379 [inlined]
 [7] promote
   @ ./promotion.jl:404 [inlined]
 [8] lcm(a::Int8, b::UInt16)
   @ Main ./REPL[6]:1
 [9] top-level scope
   @ REPL[15]:1

julia> gcd(typemin(Int8), UInt16(256))  # untouched, current behaviour
ERROR: InexactError: trunc(UInt16, -128)
Stacktrace:
 [1] throw_inexacterror(func::Symbol, to::Type, val::Int8)
   @ Core ./boot.jl:866
 [2] check_sign_bit
   @ ./boot.jl:872 [inlined]
 [3] toUInt16
   @ ./boot.jl:958 [inlined]
 [4] UInt16
   @ ./boot.jl:1011 [inlined]
 [5] convert
   @ ./number.jl:7 [inlined]
 [6] _promote
   @ ./promotion.jl:379 [inlined]
 [7] promote
   @ ./promotion.jl:404 [inlined]
 [8] gcd(a::Int8, b::UInt16)
   @ Base ./intfuncs.jl:150
 [9] top-level scope
   @ REPL[16]:1

julia> gcdx(typemin(Int8), UInt16(256))
(0x0080, 0xffff, 0x0000)

Adding support for the typemin argument in gcd and lcm with mixed signed/unsigned inputs would incur a (probably small) performance penalty but I don't want that discussion to interfere with the bugfix here, so it will be for another PR. I added it as a @test_broken instead.

Irrespective of this PR, there is of course still the following discrepancy on non-mixed types:

julia> gcdx(typemin(Int8), typemin(Int8))  # violate the condition "d is positive" from the documentation
(-128, 0, -1)

julia> gcdx(typemin(Int8), Int8(0))        # violate the condition "d is positive" from the documentation
(-128, -1, 0)

julia> gcd(typemin(Int8), typemin(Int8))
ERROR: OverflowError: gcd(-128, -128) overflows
Stacktrace:
 [1] __throw_gcd_overflow(a::Int8, b::Int8)
   @ Base .\intfuncs.jl:65
 [2] gcd(a::Int8, b::Int8)
   @ Base .\intfuncs.jl:58
 [3] top-level scope
   @ REPL[3]:1

julia> gcd(typemin(Int8), Int8(0))
ERROR: OverflowError: checked arithmetic: cannot compute |x| for x = -128::Int8
Stacktrace:
 [1] checked_abs
   @ .\checked.jl:128 [inlined]
 [2] gcd(a::Int8, b::Int8)
   @ Base .\intfuncs.jl:55
 [3] top-level scope
   @ REPL[5]:1

but again, I'd rather keep it separate from the bugfix here.

@ViralBShah ViralBShah added the backport 1.12 Change should be backported to release-1.12 label Sep 24, 2025
@adienes
Copy link
Member

adienes commented Sep 24, 2025

I'd rather keep it separate from the bugfix here.

fair enough. I do believe this is a strict improvement; I can't find any test cases where erroring-ness changes from master, and everywhere the value changes it correctly matches gcd

@adienes adienes merged commit 4f1e471 into JuliaLang:master Sep 25, 2025
9 checks passed
KristofferC pushed a commit that referenced this pull request Sep 30, 2025
Add `gcdx(a::Signed, b::Unsigned)` and `gcdx(a::Unsigned, b::Signed)`
methods to fix #58025:
```julia
julia> gcdx(UInt16(100), Int8(-101))  # pr
(0x0001, 0xffff, 0xffff)

julia> gcdx(UInt16(100), Int8(-101))  # master, incorrect result
(0x0005, 0xf855, 0x0003)
```

Also add the equivalent methods for `lcm` to fix the systematic
`InexactError` when one argument is a negative `Signed` and the other is
any `Unsigned`:
```julia
julia> lcm(UInt16(100), Int8(-101))  # pr
0x2774

julia> lcm(UInt16(100), Int8(-101))  # master, error
ERROR: InexactError: trunc(UInt16, -101)
Stacktrace:
 [1] throw_inexacterror(func::Symbol, to::Type, val::Int8)
   @ Core ./boot.jl:866
 [2] check_sign_bit
   @ ./boot.jl:872 [inlined]
 [3] toUInt16
   @ ./boot.jl:958 [inlined]
 [4] UInt16
   @ ./boot.jl:1011 [inlined]
 [5] convert
   @ ./number.jl:7 [inlined]
 [6] _promote
   @ ./promotion.jl:379 [inlined]
 [7] promote
   @ ./promotion.jl:404 [inlined]
 [8] lcm(a::UInt16, b::Int8)
   @ Base ./intfuncs.jl:152
 [9] top-level scope
   @ REPL[62]:1
```

Inspired by
#59487 (comment).
The difference is that the solution proposed in this PR keeps the
current correct result type for inputs such as `(::Int16, ::UInt8)`.

(cherry picked from commit 4f1e471)
KristofferC pushed a commit that referenced this pull request Sep 30, 2025
Add `gcdx(a::Signed, b::Unsigned)` and `gcdx(a::Unsigned, b::Signed)`
methods to fix #58025:
```julia
julia> gcdx(UInt16(100), Int8(-101))  # pr
(0x0001, 0xffff, 0xffff)

julia> gcdx(UInt16(100), Int8(-101))  # master, incorrect result
(0x0005, 0xf855, 0x0003)
```

Also add the equivalent methods for `lcm` to fix the systematic
`InexactError` when one argument is a negative `Signed` and the other is
any `Unsigned`:
```julia
julia> lcm(UInt16(100), Int8(-101))  # pr
0x2774

julia> lcm(UInt16(100), Int8(-101))  # master, error
ERROR: InexactError: trunc(UInt16, -101)
Stacktrace:
 [1] throw_inexacterror(func::Symbol, to::Type, val::Int8)
   @ Core ./boot.jl:866
 [2] check_sign_bit
   @ ./boot.jl:872 [inlined]
 [3] toUInt16
   @ ./boot.jl:958 [inlined]
 [4] UInt16
   @ ./boot.jl:1011 [inlined]
 [5] convert
   @ ./number.jl:7 [inlined]
 [6] _promote
   @ ./promotion.jl:379 [inlined]
 [7] promote
   @ ./promotion.jl:404 [inlined]
 [8] lcm(a::UInt16, b::Int8)
   @ Base ./intfuncs.jl:152
 [9] top-level scope
   @ REPL[62]:1
```

Inspired by
#59487 (comment).
The difference is that the solution proposed in this PR keeps the
current correct result type for inputs such as `(::Int16, ::UInt8)`.

(cherry picked from commit 4f1e471)
@KristofferC KristofferC mentioned this pull request Sep 30, 2025
44 tasks
KristofferC pushed a commit that referenced this pull request Sep 30, 2025
Add `gcdx(a::Signed, b::Unsigned)` and `gcdx(a::Unsigned, b::Signed)`
methods to fix #58025:
```julia
julia> gcdx(UInt16(100), Int8(-101))  # pr
(0x0001, 0xffff, 0xffff)

julia> gcdx(UInt16(100), Int8(-101))  # master, incorrect result
(0x0005, 0xf855, 0x0003)
```

Also add the equivalent methods for `lcm` to fix the systematic
`InexactError` when one argument is a negative `Signed` and the other is
any `Unsigned`:
```julia
julia> lcm(UInt16(100), Int8(-101))  # pr
0x2774

julia> lcm(UInt16(100), Int8(-101))  # master, error
ERROR: InexactError: trunc(UInt16, -101)
Stacktrace:
 [1] throw_inexacterror(func::Symbol, to::Type, val::Int8)
   @ Core ./boot.jl:866
 [2] check_sign_bit
   @ ./boot.jl:872 [inlined]
 [3] toUInt16
   @ ./boot.jl:958 [inlined]
 [4] UInt16
   @ ./boot.jl:1011 [inlined]
 [5] convert
   @ ./number.jl:7 [inlined]
 [6] _promote
   @ ./promotion.jl:379 [inlined]
 [7] promote
   @ ./promotion.jl:404 [inlined]
 [8] lcm(a::UInt16, b::Int8)
   @ Base ./intfuncs.jl:152
 [9] top-level scope
   @ REPL[62]:1
```

Inspired by
#59487 (comment).
The difference is that the solution proposed in this PR keeps the
current correct result type for inputs such as `(::Int16, ::UInt8)`.

(cherry picked from commit 4f1e471)
xal-0 pushed a commit to xal-0/julia that referenced this pull request Sep 30, 2025
Add `gcdx(a::Signed, b::Unsigned)` and `gcdx(a::Unsigned, b::Signed)`
methods to fix JuliaLang#58025:
```julia
julia> gcdx(UInt16(100), Int8(-101))  # pr
(0x0001, 0xffff, 0xffff)

julia> gcdx(UInt16(100), Int8(-101))  # master, incorrect result
(0x0005, 0xf855, 0x0003)
```

Also add the equivalent methods for `lcm` to fix the systematic
`InexactError` when one argument is a negative `Signed` and the other is
any `Unsigned`:
```julia
julia> lcm(UInt16(100), Int8(-101))  # pr
0x2774

julia> lcm(UInt16(100), Int8(-101))  # master, error
ERROR: InexactError: trunc(UInt16, -101)
Stacktrace:
 [1] throw_inexacterror(func::Symbol, to::Type, val::Int8)
   @ Core ./boot.jl:866
 [2] check_sign_bit
   @ ./boot.jl:872 [inlined]
 [3] toUInt16
   @ ./boot.jl:958 [inlined]
 [4] UInt16
   @ ./boot.jl:1011 [inlined]
 [5] convert
   @ ./number.jl:7 [inlined]
 [6] _promote
   @ ./promotion.jl:379 [inlined]
 [7] promote
   @ ./promotion.jl:404 [inlined]
 [8] lcm(a::UInt16, b::Int8)
   @ Base ./intfuncs.jl:152
 [9] top-level scope
   @ REPL[62]:1
```

Inspired by
JuliaLang#59487 (comment).
The difference is that the solution proposed in this PR keeps the
current correct result type for inputs such as `(::Int16, ::UInt8)`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 1.12 Change should be backported to release-1.12 bugfix This change fixes an existing bug maths Mathematical functions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

gcd(a,b) != gcdx(a,b)[1]
4 participants