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

Remove pointless calls to *iter() and iter_mut() #26190

Merged
merged 2 commits into from
Jun 11, 2015

Conversation

Veedrac
Copy link
Contributor

@Veedrac Veedrac commented Jun 10, 2015

Pull request for #26188.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@steveklabnik
Copy link
Member

🎊

@@ -73,7 +73,7 @@ r##"<!DOCTYPE html>

try!(write!(&mut output_file, "<h1>Rust Compiler Error Index</h1>\n"));

for (err_code, info) in err_map.iter() {
for (err_code, info) in err_map {
Copy link
Member

Choose a reason for hiding this comment

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

This kind of thing is extra interesting, because this looks like a change in semantics, but since err_map is a reference, it's not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMHO, this isn't particularly worrisome because the default - move if it's a value or take references if it's a reference - is always either the right thing or a compiler error. YMMV.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I don't think it's a problem at all, just an interesting side-effect of looking at the diff.

@alexcrichton
Copy link
Member

Thanks for the PR @Veedrac! I personally wouldn't so aggressively remove calls to iterators, as sometimes it's more clear (for example where the replacement uses &*). Could you revert the replacements to &* to keep using iterators?

In general I personally prefer for x in foo.iter() over for x in &foo as well, but it's definitely a matter of personal taste :). I don't think we're in a position to eliminate calls to iter(), though, so we don't necessarily need to aggressively try to prune them.

@Veedrac
Copy link
Contributor Author

Veedrac commented Jun 10, 2015

There are a few cases here

  1. .zip(pats.iter()).zip(&*pats)
  2. .zip(pats.iter()).zip(&pats)
  3. .zip(pats[a..b].iter()).zip(&pats[a..b]) (special-case of the prior)
  4. .zip(pats.iter_mut()).zip(&mut pats)
  5. for pat in pats.iter()for pat in &*pats
  6. for pat in pats.iter()for pat in &pats
  7. for pat in pats[a..b].iter()for pat in &pats[a..b] (special-case of the prior)
  8. for pat in pats.iter_mut()for pat in &mut pats

Can I get a definitive statement of which of these should be removed? It's not much hassle, but I'd nevertheless like to do it only once. :)

@alexcrichton
Copy link
Member

I would go for .iter() over &*, but if there's only one & then it's fine to leave as-is.

@steveklabnik
Copy link
Member

Same.

@Veedrac
Copy link
Contributor Author

Veedrac commented Jun 11, 2015

Reborrows actually seemed to be more common than calling .iter(), so this new commit changes more code than before.

FWIW, I've included reborrows of the form &**foo, which this changes to foo.iter().

@Veedrac
Copy link
Contributor Author

Veedrac commented Jun 11, 2015

I just stumbled on #21830, which changed several for x in y.iter() to for x in &*y. Not sure what the best course of action is.

@alexcrichton
Copy link
Member

This all looks great to me, thanks @Veedrac! It's sometimes tough to give a close review to all changed code in large patches like #21830, so I think these cases just fell through the cracks.

@bors: r+ d7f5fa4

@bors
Copy link
Contributor

bors commented Jun 11, 2015

⌛ Testing commit d7f5fa4 with merge b5b3a99...

bors added a commit that referenced this pull request Jun 11, 2015
@alexcrichton
Copy link
Member

This passed all builds except one mac build which failed b/c the bot disappeared, so merging manually.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants