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

repr-type specific oflo checks for enum discrim values. #23841

Closed

Conversation

pnkfelix
Copy link
Member

repr-type specific oflo checks for enum discrim values.

Moved such overflow checking into one place (in rustc::middle::ty, since it needs to be run on-demand during const_eval in some scenarios), and revised rustc_typeck accordingly.

Fix #23030

Fix #23221

Fix #23235

@rust-highfive
Copy link
Collaborator

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@pnkfelix
Copy link
Member Author

(it would be nice to get this in for the beta release.)

let prev_val = repr_type.disr_string(prev_val);
let repr_type = repr_type.to_ty(cx).user_string(cx);
span_err!(cx.sess, variant_span, E0368,
"Discriminant overflowed on value after {}: {}! \
Copy link
Member

Choose a reason for hiding this comment

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

Conventionally I believe our error messages start with lowercase letters, and I also think that two clauses are separated by semicolons (minor though)

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, the capitalization and exclamation point were both inherited from the previous checking, but I'm happy to revise.

Copy link
Member

Choose a reason for hiding this comment

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

Ah it's not super important in that case, we could just tweak the error message later if that's easier.

@alexcrichton
Copy link
Member

Looks good to me, thanks @pnkfelix! r=me with just a few nits.

@pnkfelix pnkfelix force-pushed the check-discrim-oflo-issue-23030 branch 2 times, most recently from 995b0fa to cb8957e Compare March 30, 2015 17:29
@pnkfelix
Copy link
Member Author

@bors r=alexcrichton

@bors
Copy link
Contributor

bors commented Mar 30, 2015

📌 Commit cb8957e has been approved by alexcrichton

@pnkfelix
Copy link
Member Author

@bors r-

(Rebase was unfinished)

@pnkfelix
Copy link
Member Author

@bors r=alexcrichton

@bors
Copy link
Contributor

bors commented Mar 30, 2015

📌 Commit 3800bb0 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Mar 30, 2015

⌛ Testing commit 3800bb0 with merge 2a65f72...

@bors
Copy link
Contributor

bors commented Mar 30, 2015

💔 Test failed - auto-linux-64-x-android-t

@pnkfelix
Copy link
Member Author

Okay, this reproduces locally (all one needs to do is cross-compile from a 64-bit host to a 32-bit target).

@pnkfelix
Copy link
Member Author

@bors r=alexcrichton

(I'm also going to try to rebase #23863 on top of this; if I finish that before this lands then I will just land them all at once.)

@bors
Copy link
Contributor

bors commented Mar 31, 2015

📌 Commit 3ced9d1 has been approved by alexcrichton

@pnkfelix
Copy link
Member Author

(does not fix #20600. :( more to do clearly.)

@bors
Copy link
Contributor

bors commented Mar 31, 2015

☔ The latest upstream changes (presumably #23549) made this pull request unmergeable. Please resolve the merge conflicts.

@pnkfelix pnkfelix force-pushed the check-discrim-oflo-issue-23030 branch from 3ced9d1 to 18e4e30 Compare March 31, 2015 20:47
…values.

Moved such overflow checking into one place (in `rustc::middle::ty`,
since it needs to be run on-demand during `const_eval` in some
scenarios), and revised `rustc_typeck` accordingly.

(Note that we only check for overflow if program did not provide a
discriminant value explicitly.)

Fix rust-lang#23030

Fix rust-lang#23221

Fix rust-lang#23235
@pnkfelix pnkfelix force-pushed the check-discrim-oflo-issue-23030 branch from 18e4e30 to 278227e Compare March 31, 2015 20:48
@pnkfelix
Copy link
Member Author

@bors r=alexcrichton

@bors
Copy link
Contributor

bors commented Mar 31, 2015

📌 Commit 278227e has been approved by alexcrichton

@pnkfelix
Copy link
Member Author

@bors r- (seems like my rebase was not complete)

@pnkfelix
Copy link
Member Author

pnkfelix commented Apr 1, 2015

closing in favor of #23863

@pnkfelix pnkfelix closed this Apr 1, 2015
alexcrichton added a commit to alexcrichton/rust that referenced this pull request Apr 1, 2015
const_eval : add overflow-checking for {`+`, `-`, `*`, `/`, `<<`, `>>`}.

One tricky detail here: There is some duplication of labor between `rustc::middle::const_eval` and `rustc_trans::trans::consts`. It might be good to explore ways to try to factor out the common structure to the two passes (by abstracting over the particular value-representation used in the compile-time interpreter).

----

Update: Rebased atop rust-lang#23841

Fix rust-lang#22531

Fix rust-lang#23030

Fix rust-lang#23221

Fix rust-lang#23235
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants