Turn flow::base and friends into methods #19565

mbrubeck · 2017-12-14T17:06:34Z

This feels more idiomatic in modern Rust, and replaces code like this:

flow::base(&**root_flow).restyle_damage

with this:

root_flow.base().restyle_damage.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes do not require tests because they are refactoring only

This change is

highfive · 2017-12-14T17:06:38Z

Heads up! This PR modifies the following files:

@emilio: components/layout/construct.rs, components/layout/fragment.rs, components/layout/traversal.rs, components/layout/incremental.rs, components/layout/sequential.rs and 14 more

highfive · 2017-12-14T17:06:40Z

Warning

These commits modify unsafe code. Please review it carefully!
These commits modify layout code, but no tests are modified. Please consider adding a test!

mbrubeck · 2017-12-14T17:12:22Z

cc @pcwalton

You wrote in a 2013 comment here that adding "virtual methods" to the Flow trait has a cost. Can you elaborate on that? Is it still true today?

servo/components/layout/flow.rs

Line 81 in 26feea3

    
           /// Note that virtual methods have a cost; we should not overuse them in Servo. Consider adding

emilio · 2017-12-14T17:16:38Z

Well, you pay the virtual call, I suppose...

mbrubeck · 2017-12-14T17:31:02Z

Well, you pay the virtual call, I suppose...

Oh right, while calling flow::base on an &Flow argument isn't virtual, because it operates directly on the data pointer (and ignores the vtable pointer). Hmm.

mbrubeck · 2017-12-14T18:10:16Z

Moved these methods to a separate trait that is implemented directly on both sized and unsized types, to avoid virtual calls.


        Turn flow::base and friends into methods

KiChjang · 2017-12-14T22:49:01Z

components/layout/flow.rs

@@ -76,6 +76,30 @@ use table_wrapper::TableWrapperFlow;
 #[allow(unsafe_code)]
 pub unsafe trait HasBaseFlow {}

+/// Methods to get the `BaseFlow` from any `HasBaseFlow` type.
+pub trait GetBaseFlow {


Why not let HasBaseFlow become a super trait of GetBaseFlow instead?

Making this trait GetBaseFlow: HasBaseFlow works, but I'm not sure if it adds anything. There's no technical reason for this trait to require HasBaseFlow for all impls.

emilio · 2017-12-15T17:15:07Z

@bors-servo r+

bors-servo · 2017-12-15T17:15:08Z

📌 Commit c60cfc5 has been approved by emilio

bors-servo · 2017-12-15T17:15:12Z

⌛ Testing commit c60cfc5 with merge 072029a...


        Auto merge of #19565 - mbrubeck:base, r=emilio

Turn flow::base and friends into methods This feels more idiomatic in modern Rust, and replaces code like this: `flow::base(&**root_flow).restyle_damage` with this: `root_flow.base().restyle_damage`. --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes do not require tests because they are refactoring only  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19565)

bors-servo · 2017-12-15T17:28:13Z

💔 Test failed - mac-dev-unit

mbrubeck · 2017-12-15T17:56:23Z

@bors-servo retry

#19495

bors-servo · 2017-12-15T18:44:51Z

⌛ Testing commit c60cfc5 with merge 53968fe...


        Auto merge of #19565 - mbrubeck:base, r=emilio

Turn flow::base and friends into methods This feels more idiomatic in modern Rust, and replaces code like this: `flow::base(&**root_flow).restyle_damage` with this: `root_flow.base().restyle_damage`. --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes do not require tests because they are refactoring only  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19565)

bors-servo · 2017-12-15T19:00:08Z

💔 Test failed - mac-rel-wpt3

mbrubeck · 2017-12-15T20:07:02Z

@bors-servo retry

#19495

bors-servo · 2017-12-15T20:07:04Z

⚡ Previous build results for android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt4, windows-msvc-dev are reusable. Rebuilding only mac-rel-wpt3...

bors-servo · 2017-12-15T20:55:46Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: emilio
Pushing 53968fe to master...

highfive assigned metajack Dec 14, 2017

highfive added the S-awaiting-review label Dec 14, 2017

mbrubeck force-pushed the mbrubeck:base branch from 7da484d to 9f4a00d Dec 14, 2017

mbrubeck changed the title ~~Turn flow::base and friends into Flow trait methods~~ Turn flow::base and friends into methods Dec 14, 2017

mbrubeck force-pushed the mbrubeck:base branch from 9f4a00d to f341d65 Dec 14, 2017

Turn flow::base and friends into methods

Loading status checks…

c60cfc5

mbrubeck force-pushed the mbrubeck:base branch from f341d65 to c60cfc5 Dec 14, 2017

KiChjang reviewed Dec 14, 2017

View changes

highfive assigned emilio and unassigned metajack Dec 15, 2017

highfive added S-awaiting-merge and removed S-awaiting-review labels Dec 15, 2017

highfive added S-tests-failed and removed S-awaiting-merge labels Dec 15, 2017

highfive added S-awaiting-merge and removed S-tests-failed labels Dec 15, 2017

highfive added S-tests-failed and removed S-awaiting-merge labels Dec 15, 2017

bors-servo merged commit c60cfc5 into servo:master Dec 15, 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

bors-servo mentioned this pull request Dec 15, 2017

Replace mpsc with crossbeam-channel #19515

Closed

servo / servo

Turn flow::base and friends into methods #19565

Turn flow::base and friends into methods #19565

mbrubeck commented Dec 14, 2017 •

edited by SimonSapin

highfive commented Dec 14, 2017

highfive commented Dec 14, 2017

mbrubeck commented Dec 14, 2017

emilio commented Dec 14, 2017

mbrubeck commented Dec 14, 2017

mbrubeck commented Dec 14, 2017

This comment has been minimized.

This comment has been minimized.

emilio commented Dec 15, 2017

bors-servo commented Dec 15, 2017

bors-servo commented Dec 15, 2017

bors-servo commented Dec 15, 2017

mbrubeck commented Dec 15, 2017

bors-servo commented Dec 15, 2017

bors-servo commented Dec 15, 2017

mbrubeck commented Dec 15, 2017

bors-servo commented Dec 15, 2017

bors-servo commented Dec 15, 2017

servo / servo

Join GitHub today

Turn flow::base and friends into methods #19565

Turn flow::base and friends into methods #19565

Conversation

mbrubeck commented Dec 14, 2017 • edited by SimonSapin

highfive commented Dec 14, 2017

highfive commented Dec 14, 2017

mbrubeck commented Dec 14, 2017

emilio commented Dec 14, 2017

mbrubeck commented Dec 14, 2017

mbrubeck commented Dec 14, 2017

This comment has been minimized.

KiChjang Dec 14, 2017

This comment has been minimized.

mbrubeck Dec 15, 2017

emilio commented Dec 15, 2017

bors-servo commented Dec 15, 2017

bors-servo commented Dec 15, 2017

bors-servo commented Dec 15, 2017

mbrubeck commented Dec 15, 2017

bors-servo commented Dec 15, 2017

bors-servo commented Dec 15, 2017

mbrubeck commented Dec 15, 2017

bors-servo commented Dec 15, 2017

bors-servo commented Dec 15, 2017

mbrubeck commented Dec 14, 2017 •

edited by SimonSapin