Favor the function over the method syntax when cloning a reference counted pointer. #1037

nical · 2017-03-30T12:43:38Z

data.clone() reads like performance hazard, due to the vague definition of Clone, which can be either very expensive if data is an uniquely owned large resource or very cheap if it is a reference counted pointer to such a resource. This has already confused people reading the code and even code reviews several times in webrender, so I would like to work around this unfortunate ambiguity.

I first tried to address this at the standard library level but the decision of the rust team appears to be that we should use the function syntax instead of the method syntax to address the problem.

See also the clippy issue about providing a lint for this.

I decided to take a systematic approach and use the function call syntax for all of the outstanding reference counted pointer clones that I came across (I would not be surprised that I missed some of them). Rather than try to separate the obvious from the ambiguous ones. I would rather make this a default policy than have to debate about where the line is in each code review.

The three commits are independent and can be reviewed separately (although they are small and simple enough to look at as one diff if you prefer).

This change is

nical · 2017-03-30T12:43:49Z

r? @kvark

nox · 2017-03-30T12:46:32Z

webrender/src/resource_cache.rs

@@ -709,7 +709,7 @@ impl ResourceCache {
                        is_opaque: image_descriptor.is_opaque,
                    }
                } else {
-                    image_template.descriptor.clone()
+                    image_template.descriptor


This change is not related to what the PR describes, please remove or put in its own commit.

@nox ~~I think you are overly sensitive here.~~ It doesn't appear to me that the change bears any danger, and it's a bit related to the clone() refactor.

kvark · 2017-03-30T13:05:57Z

@nical I see where you are coming from, and the change looks clean.
However, I'm not yet convinced that this is the way to go for all Rust libraries. The RFC discussion basically concluded that if you want it, you are free to use it, and the clippy issue discussion is empty so far (I know, the issue is recent). I'd like to get some opinion from @glennw and Rust Core team.

bors-servo · 2017-03-30T13:44:53Z

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

glennw · 2017-03-30T19:34:54Z

I really like this idea.


        Favor Arc::clone function syntax instead of method syntax in resource…

…_cache.rs.


        Favor Arc::clone function syntax instead of method syntax in render_b…

…ackend.rs and renderer.rs.


        Favor Rc::clone function syntax instead of method syntax in device.rs.

nical · 2017-03-30T20:38:07Z

I did a last rebase before I disappear for 2 weeks. Don't hesitate to re-submit this in my place if a consensus is approached while I'm away.
I really think we would benefit from favoring this syntax. The rust core team decided stand on the side of letting people use the function syntax if they want to. Whether we want to hand pick places where we use it or apply it consistently is really up to us. Clippy support would be a plus but doesn't affect whether this style is to be considered good or bad for us.

kvark · 2017-04-03T19:43:02Z

I'm not going to block this. @glennw please proceed if you are happy about it.

glennw · 2017-04-03T20:17:17Z

OK, let's merge it. We can always change back if we decide it's not worthwhile. Thanks!

glennw · 2017-04-03T20:17:23Z

@bors-servo r+

bors-servo · 2017-04-03T20:17:24Z

📌 Commit 45a2fa8 has been approved by glennw

bors-servo · 2017-04-03T20:24:14Z

⌛ Testing commit 45a2fa8 with merge 0d4041b...


        Auto merge of #1037 - nical:arc-clone-syntax, r=glennw

Favor the function over the method syntax when cloning a reference counted pointer. ```data.clone()``` reads like performance hazard, due to the vague definition of Clone, which can be either very expensive if data is an uniquely owned large resource or very cheap if it is a reference counted pointer to such a resource. This has already confused people reading the code and even code reviews several times in webrender, so I would like to work around this unfortunate ambiguity. I first tried to address this at the [standard library level](rust-lang/rfcs#1954) but the decision of the rust team appears to be that we should use the function syntax instead of the method syntax to address the problem. See also the [clippy issue](https://github.com/Manishearth/rust-clippy/issues/1645) about providing a lint for this. I decided to take a systematic approach and use the function call syntax for all of the outstanding reference counted pointer clones that I came across (I would not be surprised that I missed some of them). Rather than try to separate the obvious from the ambiguous ones. I would rather make this a default policy than have to debate about where the line is in each code review. The three commits are independent and can be reviewed separately (although they are small and simple enough to look at as one diff if you prefer).  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/1037)

bors-servo · 2017-04-03T21:11:16Z

☀️ Test successful - status-travis
Approved by: glennw
Pushing 0d4041b to master...

nox reviewed Mar 30, 2017

View changes

nical added 3 commits Mar 30, 2017

Favor Arc::clone function syntax instead of method syntax in resource…

9355112

…_cache.rs.

Favor Arc::clone function syntax instead of method syntax in render_b…

da915a5

…ackend.rs and renderer.rs.

Favor Rc::clone function syntax instead of method syntax in device.rs.

Loading status checks…

45a2fa8

nical force-pushed the nical:arc-clone-syntax branch from 2d7611b to 45a2fa8 Mar 30, 2017

bors-servo merged commit 45a2fa8 into servo:master Apr 3, 2017
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

servo / webrender

Favor the function over the method syntax when cloning a reference counted pointer. #1037

Favor the function over the method syntax when cloning a reference counted pointer. #1037

nical commented Mar 30, 2017 •

edited by larsbergstrom

nical commented Mar 30, 2017

This comment has been minimized.

This comment has been minimized.

kvark commented Mar 30, 2017

bors-servo commented Mar 30, 2017

glennw commented Mar 30, 2017

nical commented Mar 30, 2017

kvark commented Apr 3, 2017

glennw commented Apr 3, 2017

glennw commented Apr 3, 2017

bors-servo commented Apr 3, 2017

bors-servo commented Apr 3, 2017

bors-servo commented Apr 3, 2017

servo / webrender

Join GitHub today

Favor the function over the method syntax when cloning a reference counted pointer. #1037

Favor the function over the method syntax when cloning a reference counted pointer. #1037

Conversation

nical commented Mar 30, 2017 • edited by larsbergstrom

nical commented Mar 30, 2017

This comment has been minimized.

nox Mar 30, 2017 • edited

This comment has been minimized.

kvark Mar 30, 2017 • edited

kvark commented Mar 30, 2017

bors-servo commented Mar 30, 2017

glennw commented Mar 30, 2017

nical commented Mar 30, 2017

kvark commented Apr 3, 2017

glennw commented Apr 3, 2017

glennw commented Apr 3, 2017

bors-servo commented Apr 3, 2017

bors-servo commented Apr 3, 2017

bors-servo commented Apr 3, 2017

nical commented Mar 30, 2017 •

edited by larsbergstrom

nox Mar 30, 2017 •

edited

kvark Mar 30, 2017 •

edited