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.

Sign up for GitHub

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

Jump to bottom

Remove unwrap_none/expect_none from compiler/. #82759

Merged
merged 1 commit into from
Mar 19, 2021
Merged

Remove unwrap_none/expect_none from compiler/. #82759

merged 1 commit into from
Mar 19, 2021

Conversation

m-ou-se
Copy link
Member

@m-ou-se m-ou-se commented Mar 4, 2021

We're not going to stabilize Option::{unwrap_none, expect_none}. (See #62633.) This removes the usage of those unstable methods from compiler/.

@m-ou-se m-ou-se added C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 4, 2021
@rust-highfive
Copy link
Collaborator

r? @petrochenkov

(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 Mar 4, 2021
@petrochenkov
Copy link
Contributor

Could you avoid putting anything with side effects into asserts?
Asserts may be casually removed or turned into debug asserts, so running meaningful code in them always triggers some level of alarm.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 4, 2021
@m-ou-se m-ou-se force-pushed the remove-unwrap-none branch from 9717608 to f42beeb Compare March 8, 2021 18:44
@m-ou-se
Copy link
Member Author

m-ou-se commented Mar 8, 2021

Done.

I added try_insert (#82764) and assert_matches (#82770) to make it a bit nicer.

@m-ou-se m-ou-se added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 8, 2021
@petrochenkov
Copy link
Contributor

Could you avoid evaluating things twice as well?

foo(map[key]);
assert_eq!(map[key], expected);

=>

let value = map[key];
foo(value);
assert_eq!(value, expected);

It's not immediately obvious whether it's something cheap like array access or more expensive like BTreeMap search in each specific case.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 9, 2021
@bors

This comment has been minimized.

Sign in to view
@m-ou-se m-ou-se force-pushed the remove-unwrap-none branch from c47ad4c to 57f179d Compare March 14, 2021 11:51
@m-ou-se
Copy link
Member Author

m-ou-se commented Mar 14, 2021

Could you avoid evaluating things twice as well?

Done.

@petrochenkov
Copy link
Contributor

r=me with commits squashed.

@bors
Copy link
Contributor

bors commented Mar 18, 2021

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

@m-ou-se m-ou-se force-pushed the remove-unwrap-none branch 2 times, most recently from cdd9c5f to cfb4ad4 Compare March 18, 2021 13:25
@m-ou-se
Copy link
Member Author

m-ou-se commented Mar 18, 2021

@bors r=petrochenkov

@bors
Copy link
Contributor

bors commented Mar 18, 2021

📌 Commit cfb4ad4 has been approved by petrochenkov

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 18, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Mar 18, 2021
@Dylan-DPC
Rollup merge of rust-lang#82759 - m-ou-se:remove-unwrap-none, r=petro…
aea804f 
…chenkov

Remove unwrap_none/expect_none from compiler/.

We're not going to stabilize `Option::{unwrap_none, expect_none}`. (See rust-lang#62633.) This removes the usage of those unstable methods from `compiler/`.
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request Mar 18, 2021
Closed
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Mar 18, 2021
@Dylan-DPC
Rollup merge of rust-lang#82759 - m-ou-se:remove-unwrap-none, r=petro…
05984bc 
…chenkov

Remove unwrap_none/expect_none from compiler/.

We're not going to stabilize `Option::{unwrap_none, expect_none}`. (See rust-lang#62633.) This removes the usage of those unstable methods from `compiler/`.
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request Mar 18, 2021
Closed
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Mar 19, 2021
@Dylan-DPC
Rollup merge of rust-lang#82759 - m-ou-se:remove-unwrap-none, r=petro…
25fc5f0 
…chenkov

Remove unwrap_none/expect_none from compiler/.

We're not going to stabilize `Option::{unwrap_none, expect_none}`. (See rust-lang#62633.) This removes the usage of those unstable methods from `compiler/`.
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request Mar 19, 2021
Closed
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Mar 19, 2021
@Dylan-DPC
Rollup merge of rust-lang#82759 - m-ou-se:remove-unwrap-none, r=petro…
51af30b 
…chenkov

Remove unwrap_none/expect_none from compiler/.

We're not going to stabilize `Option::{unwrap_none, expect_none}`. (See rust-lang#62633.) This removes the usage of those unstable methods from `compiler/`.
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request Mar 19, 2021
Closed
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Mar 19, 2021
@Dylan-DPC
Rollup merge of rust-lang#82759 - m-ou-se:remove-unwrap-none, r=petro…
050d228 
…chenkov

Remove unwrap_none/expect_none from compiler/.

We're not going to stabilize `Option::{unwrap_none, expect_none}`. (See rust-lang#62633.) This removes the usage of those unstable methods from `compiler/`.
This was referenced Mar 19, 2021
Closed
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 19, 2021
@bors
Auto merge of rust-lang#83301 - Dylan-DPC:rollup-x1yzvhm, r=Dylan-DPC
9f4bc3e 
Rollup of 11 pull requests

Successful merges:

 - rust-lang#82500 (Reuse `std::sys::unsupported::pipe` on `hermit`)
 - rust-lang#82759 (Remove unwrap_none/expect_none from compiler/.)
 - rust-lang#82846 (rustdoc: allow list syntax for #[doc(alias)] attributes)
 - rust-lang#82892 (Clarify docs for Read::read's return value)
 - rust-lang#83179 (Extend `proc_macro_back_compat` lint to `actix-web`)
 - rust-lang#83197 (Move some test-only code to test files)
 - rust-lang#83208 (Fix gitattibutes for old git versions)
 - rust-lang#83215 (Deprecate std::os::haiku::raw, which accidentally wasn't deprecated)
 - rust-lang#83230 (Remove unnecessary `forward_inner_docs` hack)
 - rust-lang#83236 (Upgrade memmap to memmap2)
 - rust-lang#83270 (Fix typo/inaccuracy in the documentation of Iterator::skip_while)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 2960971 into rust-lang:master Mar 19, 2021
@rustbot rustbot added this to the 1.52.0 milestone Mar 19, 2021
@dtolnay dtolnay mentioned this pull request Mar 20, 2021
Merged
@cuviper cuviper mentioned this pull request Jun 7, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petrochenkov petrochenkov petrochenkov left review comments

Assignees

@petrochenkov petrochenkov

Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. 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
Milestone
1.52.0
Development

Successfully merging this pull request may close these issues.
5 participants
@m-ou-se @rust-highfive @petrochenkov @bors @rustbot