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

Add modular operators +% -% >>% etc. #408

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jakobnissen
Copy link

@jakobnissen jakobnissen commented Jan 9, 2024

For the last ten years, there have been idle discussion about allowing a way for users to opt-in for explicitly overflowing integer operations. Julia's arithmetic operations do overflow by default, but it may in the future be possible to run Julia in a mode where overflows are detected, for testing purposes. If this is to be done, then there needs to be a way to signal that the user explicitly wants overflow, which is important in some low-level operations, such as hashing.
For bitshifting, Julias definition of e.g. >> checks for overflow, which leads to suboptimal code. Analogous to the opt-in overflowing arithmetic ops like +%, users ought to be able to opt-in to the more efficient overflowing shifts, which in situations where bitshifts are a performance bottleneck.

This impements the JuliaSyntax side of Keno's JuliaLang/julia#50790 (cc @Keno).

Note for reviewers:

  1. The tests currently fail, due to the test that the operators in JuliaSyntax matches Base.isoperator (presumably this is the flisp parser)? This is not a bug, this just means the flisp parser needs an update, too
  2. Keno has already updated the flisp parser to accept e.g. +% in WIP/RFC: Add explicitly wrapping versions of integer arithmetic julia#50790. However, in this PR I also add overflowing bitshifts to address Feature request: Native bitshift operation julia#50225 (also see its linked issues). These overflowing bitshift operators also needs support in the flisp parser. I can give a try to updating the flisp parser based on Keno's PR. The overflowing bitshifts needs support in Julia, too, but the implementation should be straightforward. I'm willing to amend WIP/RFC: Add explicitly wrapping versions of integer arithmetic julia#50790 to implement, document and test these overflowing shifts, if @Keno gives his blessings.

This is my first PR to JuliaSyntax, and I don't really understand how JuliaSyntax is vendored into Julia, so if I need to do anything special for this PR, please let me know.

For the last ten years, there have been idle discussion about allowing a way for
users to opt-in for explicitly overflowing integer operations.
Julia's arithmetic operations do overflow by default, but it may in the future be
possible to run Julia in a mode where overflows are detected, for testing
purposes. If this is to be done, then there needs to be a way to signal that the
user explicitly wants overflow, which is important in some low-level operations,
such as hashing.
For bitshifting, Julias definition of e.g. >> checks for overflow, which leads
to suboptimal code. Analogous to the opt-in overflowing arithmetic ops like +%,
users ought to be able to opt-in to the more efficient overflowing shifts, which
in situations where bitshifts are a performance bottleneck.
@savq
Copy link
Contributor

savq commented Feb 22, 2024

Is there any reason this couldn't be done with operator suffixes like +ₒ, -ₒ, >>ₒ?

(I ask because Julia doesn't seem to mind using unicode for pretty conventional functions like xor.)

@PallHaraldsson
Copy link

PallHaraldsson commented Feb 23, 2024

[EDIT: I'm not downvoting the code, I don't feel like we need new operators agree with @c42f and I guess the code is good, elsewhere for a functional-form.]

It could be but shouldn't(?) xor is the only exception to using Unicode operator only, and that's because xor in the function form is ASCII. We have a policy of having ASCII only or both, and I at least do not like operators on Unicode only or both (plus possibly function form, that's not proposed here).

[xor is rare in most code, why Unicode operator, the standard one, for xor, was considered justified, and those you proposed are not, only obvious when explained, and even then unclear how to type. While with % that's not a problem, and it's understood as modular by many.]

@PallHaraldsson
Copy link

PallHaraldsson commented Feb 23, 2024

Please don't add new operators, until we know needed, and better solutions not possible.

E.g. I'm thinking maybe we rather need to solve it with the type system. It seems modular arithmetic is most useful for Unsigned types, e.g. have MInt128 down to MInt8 corresponding to UInts. Actually (down to) at least MInt6, and even MInt3, corresponding to my idea here:

JuliaLang/julia#52828 (comment)

If we were ever to change the default types, e.g. Int[64], then those signed ones could be changed, and I'm actually unsure if anyone wants or needs them modular, except for the speed argument. The unsigned ones however need to be sometimes modular, but that's also more dangerous, but also not the default types, so we get away with it more. Maybe in the end we could just change the definition of the signed ones, not need 3 or 4 classes, i.e. adding MInt types...

@c42f
Copy link
Member

c42f commented Mar 12, 2024

The code looks pretty good here 👍

I have misgivings about special syntax for >>%... along the lines of JuliaLang/julia#52828 (comment). But regardless of that, we should have any debate about actual syntax changes upstream in JuliaLang/Julia - not that many people are paying attention to the JuliaSyntax.jl repo.

Once something is decided upstream, we can enact it here! The procedure to get it into Base is to bump https://github.com/JuliaLang/julia/blob/master/deps/JuliaSyntax.version and the new version should get vendored in there.

@c42f
Copy link
Member

c42f commented Mar 12, 2024

I've added my thoughts about the alternatives to special syntax here: JuliaLang/julia#50790 (comment)

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.

4 participants