Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Compatibility for overriding cld/fld #34134

Closed
sostock opened this issue Dec 18, 2019 · 2 comments
Closed

Compatibility for overriding cld/fld #34134

sostock opened this issue Dec 18, 2019 · 2 comments

Comments

@sostock
Copy link
Contributor

sostock commented Dec 18, 2019

The implementation on current master (see below) claims to provide compatibility for overriding cld/fld, but actually doesn’t (at least in the case of HalfIntegers.jl, which is currently broken on master for this reason).

julia/base/div.jl

Lines 223 to 233 in 8707744

# These are kept for compatibility with external packages overriding fld/cld.
# In 2.0, packages should extend div(a,b,r) instead, in which case, these can
# be removed.
fld(x::Real, y::Real) = div(promote(x,y)..., RoundDown)
cld(x::Real, y::Real) = div(promote(x,y)..., RoundUp)
fld(x::Signed, y::Unsigned) = div(x, y, RoundDown)
fld(x::Unsigned, y::Signed) = div(x, y, RoundDown)
cld(x::Signed, y::Unsigned) = div(x, y, RoundUp)
cld(x::Unsigned, y::Signed) = div(x, y, RoundUp)
fld(x::T, y::T) where {T<:Real} = throw(MethodError(div, (x, y, RoundDown)))
cld(x::T, y::T) where {T<:Real} = throw(MethodError(div, (x, y, RoundUp)))

HalfIntegers.jl defines fld(x::T, y::T) where T<:HalfInteger. Due to the changes on master, this method isn’t called anymore when fld is called with two different types of HalfIntegers. In order for the above code to fulfill its purpose, the first two and last two methods above could be replaced with

fld(x::Real, y::Real) = fld(promote(x,y)...)
cld(x::Real, y::Real) = cld(promote(x,y)...)
fld(x::T, y::T) where {T<:Real} = div(x, y, RoundDown)
cld(x::T, y::T) where {T<:Real} = div(x, y, RoundUp)
@Keno
Copy link
Member

Keno commented Dec 19, 2019

Yeah, this fallback doesn't work 100% of the time, but your suggestion doesn't work either, because it would cause dispatch loops due to the reverse fallbacks elsewhere in the code. This is the best I could come up with for the time being. Packages were quite inconsistent about how these methods were being overloaded. I'd recommend additionally defining fld(x::HalfInteger, y::HalfInteger) and the three-argument div method in your package.

@sostock
Copy link
Contributor Author

sostock commented Dec 19, 2019

Ok, if compatibility cannot be achieved for all packages, I will update my package and mark old versions as incompatible with Julia 1.4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants