Skip to content

Properly handle ranges of signed enums using both extremums (fixes #49973) #49981

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
Apr 18, 2018

Conversation

nox
Copy link
Contributor

@nox nox commented Apr 15, 2018

Fixes #49973.

@rust-highfive
Copy link
Contributor

r? @michaelwoerister

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 15, 2018
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Please add an assertion that size_of::<Option<E>>() == size_of::<E>(). I have not read the code but the use of valid_range: min..=max makes me slightly concerned that this may not be the case.

Or if that is too complicated to handle in this PR, please file an issue to follow up.

@nox
Copy link
Contributor Author

nox commented Apr 15, 2018

Please add an assertion that size_of::<Option<E>>() == size_of::<E>(). I have not read the code but the use of valid_range: min..=max makes me slightly concerned that this may not be the case.

It is indeed not the case, which is why I didn't put a test for that. It's a known issue, though, there is a todo and whatnot in the code that some enums' valid ranges are larger than necessary. I'll file an actual issue on GH linking to it.

@dtolnay
Copy link
Member

dtolnay commented Apr 15, 2018

Sounds good, thanks.

@michaelwoerister
Copy link
Member

r? @eddyb

@eddyb
Copy link
Member

eddyb commented Apr 17, 2018

@bors r+

Do we want to backport this?

@bors
Copy link
Collaborator

bors commented Apr 17, 2018

📌 Commit a7c4b5c has been approved by eddyb

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 17, 2018
@eddyb eddyb added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Apr 17, 2018
@bors
Copy link
Collaborator

bors commented Apr 18, 2018

⌛ Testing commit a7c4b5c with merge 65d201f...

bors added a commit that referenced this pull request Apr 18, 2018
Properly handle ranges of signed enums using both extremums (fixes #49973)

Fixes #49973.
@eddyb
Copy link
Member

eddyb commented Apr 18, 2018

cc @rust-lang/compiler Regarding backporting (#49981 (comment))

@bors
Copy link
Collaborator

bors commented Apr 18, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: eddyb
Pushing 65d201f to master...

@bors bors merged commit a7c4b5c into rust-lang:master Apr 18, 2018
@nox nox deleted the fix-signed-niches branch April 19, 2018 14:51
@nagisa nagisa added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 19, 2018
@pnkfelix pnkfelix added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Apr 19, 2018
@alexcrichton alexcrichton removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Apr 20, 2018
bors added a commit that referenced this pull request Apr 21, 2018
[beta] Processing merged backports

This is a backport of the following PRs:

* #49386
* #49465
* #49647
* #49692
* #49695
* #49714
* #49730
* #49830
* #49981
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants