Skip to content

cleanup: use the for x in &xs form instead of for x in xs.iter() #21830

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
Merged
merged 7 commits into from
Feb 3, 2015
Merged

cleanup: use the for x in &xs form instead of for x in xs.iter() #21830

merged 7 commits into from
Feb 3, 2015

Conversation

japaric
Copy link
Member

@japaric japaric commented Feb 1, 2015

This also implements IntoIterator for HashMaps and HashSets, and changes the conditionals for x in option.iter() statements to the more readable form: if let Some(x) = option. (The former looks like an ordinary for-loop, the later clearly states conditional execution).

r? @alexcrichton
This needs a snapshot before it can land.

@japaric japaric changed the title cleanup: use the for x &xs form instead of for x in xs.iter() cleanup: use the for x in &xs form instead of for x in xs.iter() Feb 1, 2015
@alexcrichton
Copy link
Member

changes the conditionals for x in option.iter() statements to the more readable form: if let Some(x) = option. (The former looks like an ordinary for-loop, the later clearly states conditional execution).

Nice!

@alexcrichton
Copy link
Member

cc @aturon you'd probably be quite interested in the fallout here.

This is basically r=me, just want to run it by @aturon first as well, nice work @japaric!

@japaric
Copy link
Member Author

japaric commented Feb 1, 2015

@bors: try 8faaedb

@bors
Copy link
Collaborator

bors commented Feb 1, 2015

⌛ Testing commit 8faaedb with merge f72a395...

@eddyb
Copy link
Member

eddyb commented Feb 1, 2015

😻 Glad to see those for loops used with Option gone, the only argument I've ever heard in their favor was that there's no extra indentation like with match.
All the code I've touched recently that effectively matched over an Option, with no action in the None case, I've converted to if let, be it match or for.
It's amazing how many levels of indentation you can dispose of - makes me a little sad match is the way it is, but I like its syntax. But that's already irrelevant to this PR.

@bors
Copy link
Collaborator

bors commented Feb 1, 2015

💔 Test failed - auto-mac-64-opt

@japaric
Copy link
Member Author

japaric commented Feb 1, 2015

@bors: try f78904c

@bors
Copy link
Collaborator

bors commented Feb 1, 2015

⌛ Testing commit f78904c with merge f5730a2...

@bors
Copy link
Collaborator

bors commented Feb 1, 2015

💔 Test failed - auto-win-64-nopt-t

@japaric
Copy link
Member Author

japaric commented Feb 1, 2015

@bors: try baa4ac1

@bors
Copy link
Collaborator

bors commented Feb 1, 2015

⌛ Testing commit baa4ac1 with merge 3a566cd...

@bors
Copy link
Collaborator

bors commented Feb 1, 2015

💔 Test failed - auto-mac-64-opt

@japaric
Copy link
Member Author

japaric commented Feb 1, 2015

@bors: try 6193055

@bors
Copy link
Collaborator

bors commented Feb 1, 2015

⌛ Testing commit 6193055 with merge f1be0a3...

@bors
Copy link
Collaborator

bors commented Feb 1, 2015

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-32-nopt-t, auto-win-32-opt, auto-win-64-nopt-t, auto-win-64-opt

@japaric
Copy link
Member Author

japaric commented Feb 1, 2015

Ok, this should be ready to go now. And sorry for hogging the try bot, I was sure there were some non-linux errors that I can't find with a local make check, but hopefully all of them have been fixed now!

@aturon
Copy link
Member

aturon commented Feb 1, 2015

Great work as always, @japaric!

@aturon
Copy link
Member

aturon commented Feb 1, 2015

@bors: r=alexcrichton

@bors
Copy link
Collaborator

bors commented Feb 1, 2015

🙀 You have the wrong number! Please try again with 052aa9a.

@aturon
Copy link
Member

aturon commented Feb 1, 2015

@bors: r=alexcrichton 5e7ebb1

@bors
Copy link
Collaborator

bors commented Feb 1, 2015

🙀 You have the wrong number! Please try again with 052aa9a.

@aturon
Copy link
Member

aturon commented Feb 1, 2015

Huh, there's some discrepancy with the last commit in order and the one bors seems to want. @japaric, any idea what's going on there?

@japaric
Copy link
Member Author

japaric commented Feb 1, 2015

@aturon github orders commits chronologically not topologically, the real last commit is 052aa9a

@bors: r=alexcrichton 052aa9a

@japaric
Copy link
Member Author

japaric commented Feb 2, 2015

@bors: r=alexcrichton 3484706

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Feb 2, 2015
@alexcrichton
rollup merge of rust-lang#21830: japaric/for-cleanup
7335c7d 
Conflicts:
	src/librustc/metadata/filesearch.rs
	src/librustc_back/target/mod.rs
	src/libstd/os.rs
	src/libstd/sys/windows/os.rs
	src/libsyntax/ext/tt/macro_parser.rs
	src/libsyntax/print/pprust.rs
	src/test/compile-fail/issue-2149.rs
@bors bors merged commit 3484706 into rust-lang:master Feb 3, 2015
@japaric japaric deleted the for-cleanup branch February 6, 2015 00:46
@brson
Copy link
Contributor

brson commented Feb 9, 2015

Massive patch.

@Veedrac Veedrac mentioned this pull request Jun 11, 2015
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@alexcrichton alexcrichton

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@japaric @alexcrichton @bors @eddyb @aturon @brson