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

Rely on @checked in OverflowContexts and reexport #16

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

BioTurboNick
Copy link

@BioTurboNick BioTurboNick commented May 3, 2024

As discussed in #12, merging some of the functionality of this package with OverflowContexts (https://github.com/JuliaMath/OverflowContexts.jl) is desired, as is using its name. The name is also preferred for that portion, as it would be inclusive of other modes, e.g., saturating math.

OverflowContexts. expanded the @checked macro of this package and expanded it to include other features including @unchecked, allowing these to be arbitrarily nested, and providing the capability to set a default for a module with @default_checked and @default_unchecked at the top of a module. It also fixes some limitations with the implementation here.

This PR removes the portions that are present in OverflowContexts and adds OverflowContexts as a dependency (starting with newly-registered v0.2.4).

All tests of this package passed with the dependency added and the native macro removed before I removed those specific tests here. For compatibility, CheckedArithmetic will re-exports the portions of OverflowContexts necessary for there to be no break for users of CheckedArithmetic.

@kimikage
Copy link
Collaborator

kimikage commented May 5, 2024

I do not see a problem with this change itself.
However, do we need to maintain CheckedArithmetic now?
Since CheckedArithmetic is not obviously broken, there may be no need to add a dependency until a concrete problem arises.
I just want to minimize maintenance costs.

@BioTurboNick
Copy link
Author

BioTurboNick commented May 5, 2024

JuliaHub reports 0 dependents on this or CheckedArithmeticCore, so I guess no real need.

There are a couple things in here that I didn't move over... but I guess they just map values to (U)Int128s for "safer" math? Is that functionality useful to keep around? If not, maybe deprecation is in order.

Perhaps we can add a note about OverflowContexts to the README at minimum.

@kimikage
Copy link
Collaborator

kimikage commented May 5, 2024

Perhaps we can add a note about OverflowContexts to the README at minimum.

Yes, I agree.

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.

2 participants