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

Math operations with Int does not warn overflow #44565

Closed
felipemarkson opened this issue Mar 11, 2022 · 17 comments
Closed

Math operations with Int does not warn overflow #44565

felipemarkson opened this issue Mar 11, 2022 · 17 comments

Comments

@felipemarkson
Copy link

Hi, I guess that could be a bug, but overflows shouldn't be warned?

julia> typemax(Int) + 1
-9223372036854775808
julia> typemax(Int)^2
-1
julia> typemax(Int)*2
-2

Compilation: Generic Linux on x86 64-bit
Version: Julia v1.6.5 (LTS)

@felipemarkson felipemarkson changed the title Math operations with Int does not warning overflow Math operations with Int does not warn overflow Mar 11, 2022
@oscardssmith
Copy link
Member

dup of #22539

@felipemarkson
Copy link
Author

Yeah, sure, I saw that Issue. But this shouldn't be warned?
I think this silent overflow can cause huge bugs in production projects.
Not everybody knows that you need to use BigInt or could be a function that causes the overflow and the programmer is not warned.

@felipemarkson
Copy link
Author

I could think of a way to warn that.
Should I create a PR?

@oscardssmith
Copy link
Member

The problem isn't warning, the problem is that warning without tanking performance. This is probably achievable for exponentiation, but definitely isn't for addition/multiplication.

@felipemarkson
Copy link
Author

I understand that, but could you please appoint me where the code could be changed to implement it?

I want to make some tests and benchmarks.

@felipemarkson
Copy link
Author

dup of #22539

I also think this is not a duplication of that issue.

@oscardssmith
Copy link
Member

Base/int.jl:87 for addition, and 88 for multiplication. Good luck.

@KristofferC
Copy link
Member

A warning is a problem. It should either error or do nothing, but not warn.

@felipemarkson
Copy link
Author

I don't know.
In my experience with embedded systems, you can make some deep tricks with overflows to get some performance.
But I guess Julia has not the intent to support these devices.

@felipemarkson
Copy link
Author

@oscardssmith Could you re-open this issue?
So, I can make some tests and demonstrate them here.

@oscardssmith
Copy link
Member

OK. That said, be warned that erroring (or warning) is a breaking change that can't happen until 2.0, and is unlikely to be merged unless the error has no runtime cost (which it will).

@oscardssmith oscardssmith reopened this Mar 11, 2022
@KristofferC
Copy link
Member

There is nothing really to discuss here. You can take a look at #21600 for erroring for pow.

@felipemarkson
Copy link
Author

But this shouldn't be an open issue to PRs references it?

@Seelengrab
Copy link
Contributor

Seelengrab commented Mar 11, 2022

The reason this issue was (originally) closed so quickly is because there already has been a tremendous amount of discussion about this, e.g. on discourse. Julia just isn't at that stage of development anymore, where such things are considered.

@felipemarkson
Copy link
Author

felipemarkson commented Mar 11, 2022

Okay, but this is a problem and should be documented somewhere.
Not for discussion, but so other people can consult it and learn about it.

@Seelengrab
Copy link
Contributor

Seelengrab commented Mar 11, 2022

Okay, but this is a problem and should be documented somewhere.

This is already prominently answered/documented, in the FAQ of the documentation.

@felipemarkson
Copy link
Author

Ok, thanks!

I'm closing this issue since this discussion was already done and is documented in the Integers Documentation and in the FAQ of the documentation.

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

No branches or pull requests

4 participants