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

clarify extreme operator behaviour #1237

Merged
merged 2 commits into from
Sep 18, 2015
Merged

Conversation

Gankra
Copy link
Contributor

@Gankra Gankra commented Aug 4, 2015

This is just clarifying things which were agreed on in various places but poorly specified.

rendered draft

enabled this will panic. When checking is disabled this will two's complement
wrap.
- The operations `/`, `%` are nonsensical for the arguments `INT_MIN` and `-1`.
When this occurs there is an unconditional panic.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe division and remainder by 0 are "nonsensical", but INT_MIN / -1 is an overflow, the same way -INT_MIN is (which isn't caught, not even in a debug build).

Copy link
Member

Choose a reason for hiding this comment

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

rust-lang/rust#24500 and rust-lang/rust#23154 and rust-lang/rust#22020 all seem to involve unary negation checking for overflow, but in practice this seems to be not working indeed in the context of const-eval.

Copy link
Member

Choose a reason for hiding this comment

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

-std::i32::MIN does give a const-eval error, but let m = std::i32::MIN; -m doesn't (so there's no runtime checking).

Copy link
Member

Choose a reason for hiding this comment

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

Seems to work for me.

Copy link
Member

Choose a reason for hiding this comment

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

Oops, I used playbot - does it run in release mode?

[17:13] <eddyb> playbot: let x = std::i32::MIN; -x
[17:13] [Notice] -playbot to #rust-internals- -2147483648

Copy link
Contributor

Choose a reason for hiding this comment

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

Regarding -INT_MIN and INT_MIN/-1 -- for whatever reason, division had code to check and panic, but multiplication did two's complement wrapping. We have preserved that behavior, afaik, inconsistent or not, and hence these edits correctly describe the current situation.

@nrc nrc added the T-lang Relevant to the language team, which will review and decide on the RFC. label Aug 6, 2015
@nikomatsakis nikomatsakis self-assigned this Aug 6, 2015
@Gankra
Copy link
Contributor Author

Gankra commented Aug 7, 2015

Clarified.

@glaebhoerl
Copy link
Contributor

As @bill-myers originally pointed out here INT_MIN % -1 is mathematically well-defined to be 0.

@nikomatsakis
Copy link
Contributor

Hear ye, hear ye. This RFC is entering final comment period.

@nikomatsakis
Copy link
Contributor

@glaebhoerl true. I guess we could consider altering the behavior, but I think this RFC is basically aiming at documenting existing behavior, and that particular result is long-standing.

@glaebhoerl
Copy link
Contributor

@nikomatsakis Then shall I open a separate issue to keep track of that possibility?

@nikomatsakis
Copy link
Contributor

On Sat, Sep 05, 2015 at 01:53:14AM -0700, Gábor Lehel wrote:

@nikomatsakis Then shall I open a separate issue to keep track of that possibility?

Sure.

@nikomatsakis nikomatsakis added the final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. label Sep 17, 2015
@nikomatsakis
Copy link
Contributor

Huzzah! The language design team has decided to accept this RFC.

nikomatsakis added a commit that referenced this pull request Sep 18, 2015
clarify extreme operator behaviour
@nikomatsakis nikomatsakis merged commit dd79587 into rust-lang:master Sep 18, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. T-lang Relevant to the language team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants