Skip to content

Add checked, wrapping and saturating arithmetic for rem/mod #230

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

Merged
merged 2 commits into from
Sep 19, 2020

Conversation

kimikage
Copy link
Collaborator

@kimikage kimikage commented Sep 15, 2020

The default arithmetic for rem is still checked arithmetic.

saturating_{rem|mod} behaves somewhat strangely in the case of the overflow. (cf. #221 (comment))

julia> x = typemin(Q0f7); y = -eps(Q0f7);

julia> saturating_div(x, y) # 128 --> 127
127

julia> saturating_rem(x, y) # != 0
-0.008Q0f7

julia> x == saturating_div(x, y) * y + saturating_rem(x, y)
true

This also adds the support for 3-arg rem. Only RoundToZero(default), RoundDown(mod), and RoundUp are supported. In RoundUp mode, rem causes OverflowError with unsigned (i.e. Normed) values. Otherwise, rem/mod does not cause an overflow.

This also adds dummy checked, wrapping and saturating rem for type modulus to prevent MethodError due to lexical replacement of x % X.

This completes the implementation of the checked arithmetic. Performance tweaks without changing behavior and the rest of #185 can be carried out after the release of v0.9.0.

Closes #221

The default arithmetic for `rem` is still checked arithmetic.
This also adds the support for 3-arg `rem`.
In `RoundUp` mode, `rem` causes `OverflowError` with unsigned (i.e. `Normed`) values.
Otherwise, `rem`/`mod` does not cause an overflow.
This prevents MethodError due to lexical replacement of `x % X`.
@codecov
Copy link

codecov bot commented Sep 15, 2020

Codecov Report

Merging #230 into master will increase coverage by 0.13%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #230      +/-   ##
==========================================
+ Coverage   95.42%   95.55%   +0.13%     
==========================================
  Files           6        6              
  Lines         677      698      +21     
==========================================
+ Hits          646      667      +21     
  Misses         31       31              
Impacted Files Coverage Δ
src/FixedPointNumbers.jl 96.49% <100.00%> (+0.33%) ⬆️
src/fixed.jl 97.91% <100.00%> (-0.03%) ⬇️
src/normed.jl 93.48% <100.00%> (-0.07%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c6b8a9c...e4ebc3b. Read the comment docs.

Copy link
Member

@timholy timholy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like how much you've been able centralize here, nice work as always!

@kimikage
Copy link
Collaborator Author

There is a bit more discussion about what to do with these arithmetic methods in v0.9.0, but I'll merge this first.
Although my plan is JuliaMath/CheckedArithmetic.jl#9, @timholy may have a slightly different idea (cf. https://discourse.julialang.org/t/rfc-what-should-the-arithmetic-within-the-fixedpointnumbers-be/46697/36).

@kimikage kimikage merged commit 28e72f8 into JuliaMath:master Sep 19, 2020
@kimikage kimikage deleted the checked_rem branch September 19, 2020 11:42
@kimikage kimikage mentioned this pull request Apr 30, 2024
38 tasks
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 this pull request may close these issues.

[RFC] Supporting checked arithmetic for division-related operations
2 participants