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

Prefer `.cloned()` over `.map(|foo| foo.clone())` #7946

Closed

Conversation

@pierrechevalier83
Copy link
Contributor

pierrechevalier83 commented Oct 9, 2015

There are other combinations of map and clone in the code, but
this commit only addresses the straightforward ones.

Fixes #7906.

Review on Reviewable

There are other combinations of map and clone in the code, but
this commit only addresses the straightforward ones.

Fixes 367.
@pierrechevalier83
Copy link
Contributor Author

pierrechevalier83 commented Oct 9, 2015

As mentioned in the commit message, I ignored a number of "kinda similar instances".
namely:

$ git grep -E "\.map\(\|.*\| .*\.clone\(\)\)"
components/gfx/platform/macos/font_template.rs:        ctfont.as_ref().map(|ctfont| (*ctfont).clone())
components/script/dom/eventtarget.rs:            listeners.iter().map(|entry| entry.listener.clone()).collect()
components/script/dom/eventtarget.rs:            filtered.map(|entry| entry.listener.clone()).collect()
components/script/dom/formdata.rs:        let name = filename.unwrap_or(f.map(|inner| inner.name().clone()).unwrap_or("blob".to_owned()));
components/script/dom/htmlfontelement.rs:                            .map(|value| value.as_atom().clone())
components/script/dom/htmlinputelement.rs:            .map(|name| name.value().as_atom().clone())
components/script/dom/xmlhttprequest.rs:                headers.as_ref().map(|h| *self.response_headers.borrow_mut() = h.clone());
components/util/prefs.rs:    PREFS.lock().unwrap().get(name).map(|x| x.value().clone()).unwrap_or(Arc::new(PrefValue::Missing))

Please, fail the review if you think some of these can and should also use .cloned()

@jdm
Copy link
Member

jdm commented Oct 9, 2015

Your choices look reasonable to me!

@jdm
Copy link
Member

jdm commented Oct 9, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Oct 9, 2015

📌 Commit 7286a3d has been approved by jdm

@jdm
Copy link
Member

jdm commented Oct 9, 2015

@bors-servo: r-
Thanks for the effort @pierrechevalier83 :)

@jdm jdm closed this Oct 9, 2015
@pierrechevalier83 pierrechevalier83 deleted the pierrechevalier83:fix_issue_367 branch Oct 9, 2015
@pierrechevalier83
Copy link
Contributor Author

pierrechevalier83 commented Oct 9, 2015

thanks for bearing with me...

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

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.