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

Add move protection to Root and friends #5855

Merged
merged 3 commits into from
Apr 28, 2015
Merged

Conversation

Manishearth
Copy link
Member

fixes #5724, #5737

uses https://github.com/Manishearth/rust-tenacious (can be moved in-tree if needed)

I can make it Deny by default too (I'll add a cargo feature to tenacious), though we might want it on
Warn until we get some mileage on it.

Review on Reviewable

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Apr 26, 2015
@hoppipolla-critic-bot
Copy link

Critic review: https://critic.hoppipolla.co.uk/r/4807

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

@Manishearth
Copy link
Member Author

cc @Munksgaard ExprUseVisitor was quite useful here.

However, in the case of humpty_dumpty, we need to do the opposite of ExprUseVisitor -- we can use it to track consumption (ish) but we'll still need to track branches somehow. Not sure if it will be useful; I couldn't quickly come up with a way that avoided tracking branches.

@Manishearth
Copy link
Member Author

r? @kmcallister @jdm

@bors-servo
Copy link
Contributor

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

@bors-servo
Copy link
Contributor

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

@kmcallister
Copy link
Contributor

Reviewed files:

  • components/plugins/Cargo.toml @ r1
  • components/plugins/lib.rs @ r1
  • components/script/dom/bindings/codegen/CodegenRust.py @ r2
  • components/script/dom/bindings/global.rs @ r2
  • components/script/dom/bindings/js.rs @ r3
  • components/script/dom/bindings/mod.rs @ r2
  • components/script/dom/bindings/trace.rs @ r3
  • components/script/dom/document.rs @ r2
  • components/script/dom/domimplementation.rs @ r2
  • components/script/dom/element.rs @ r2
  • components/script/dom/eventdispatcher.rs @ r2
  • components/script/dom/htmlformelement.rs @ r2
  • components/script/dom/htmlinputelement.rs @ r2
  • components/script/dom/htmllinkelement.rs @ r3
  • components/script/dom/htmlscriptelement.rs @ r2
  • components/script/dom/node.rs @ r3
  • components/script/dom/text.rs @ r2
  • components/script/script_task.rs @ r3
  • components/servo/Cargo.lock @ r2
  • ports/cef/Cargo.lock @ r2
  • ports/gonk/Cargo.lock @ r2

Comments from the review on Reviewable.io

@Manishearth
Copy link
Member Author

@bors-servo r=kmc,munksgaard

(@Munksgaard reviewed rust-tenacious)

@bors-servo
Copy link
Contributor

📌 Commit f770334 has been approved by kmc,munksgaard

@jdm jdm added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Apr 27, 2015
@bors-servo
Copy link
Contributor

⌛ Testing commit f770334 with merge 834f1e2...

bors-servo pushed a commit that referenced this pull request Apr 27, 2015
fixes #5724, #5737


uses https://github.com/Manishearth/rust-tenacious (can be moved in-tree if needed)

I can make it `Deny` by default too (I'll add a cargo feature to tenacious), though we might want it on
`Warn` until we get some mileage on it.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/5855)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

💔 Test failed - linux1

@Manishearth
Copy link
Member Author

@bors-servo: retry

@Ms2ger
Copy link
Contributor

Ms2ger commented Apr 27, 2015


/home/servo/buildbot/slave/linux2/build/components/script/dom/element.rs:731:13: 731:17 warning: #[no_move] type `dom::bindings::js::Root<dom::attr::Attr>` moved, #[warn(moved_no_move)] on by default
/home/servo/buildbot/slave/linux2/build/components/script/dom/element.rs:731         for attr in attrs.iter().map(|attr| attr.root()) {
                                                                                         ^~~~
/home/servo/buildbot/slave/linux2/build/components/script/dom/element.rs:731:13: 731:17 warning: #[no_move] type `core::option::Option<dom::bindings::js::Root<dom::attr::Attr>>` moved, #[warn(moved_no_move)] on by default
/home/servo/buildbot/slave/linux2/build/components/script/dom/element.rs:731         for attr in attrs.iter().map(|attr| attr.root()) {
                                                                                         ^~~~
/home/servo/buildbot/slave/linux2/build/components/script/dom/htmlinputelement.rs:844:18: 844:24 warning: #[no_move] type `dom::bindings::js::Root<dom::htmlinputelement::HTMLInputElement>` moved, #[warn(moved_no_move)] on by default
/home/servo/buildbot/slave/linux2/build/components/script/dom/htmlinputelement.rs:844             Some(button) => {
                                                                                                       ^~~~~~
/home/servo/buildbot/slave/linux2/build/components/script/dom/htmlinputelement.rs:844:13: 844:25 warning: #[no_move] type `core::option::Option<dom::bindings::js::Root<dom::htmlinputelement::HTMLInputElement>>` moved, #[warn(moved_no_move)] on by default
/home/servo/buildbot/slave/linux2/build/components/script/dom/htmlinputelement.rs:844             Some(button) => {
                                                                                                  ^~~~~~~~~~~~

@Manishearth
Copy link
Member Author

ick, I thought I fixed that. Must have lost it in a rebase.

@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Apr 27, 2015
@Manishearth
Copy link
Member Author

@bors-servo r=kmc,munksgaard

@bors-servo
Copy link
Contributor

📌 Commit 0000000 has been approved by kmc,munksgaard

@Manishearth
Copy link
Member Author

@bors-servo r=kmc,munksgaard

@bors-servo
Copy link
Contributor

📌 Commit 0000000 has been approved by kmc,munksgaard

@Manishearth
Copy link
Member Author

@bors-servo r=kmc,munksgaard c1b9910

@bors-servo
Copy link
Contributor

🙀 c1b9910 is not a valid commit SHA. Please try again with 0000000.

@Manishearth
Copy link
Member Author

wha

@Manishearth
Copy link
Member Author

@bors-servo r=kmc,munksgaard c1b9910

@bors-servo
Copy link
Contributor

🙀 c1b9910 is not a valid commit SHA. Please try again with 0000000.

@Manishearth
Copy link
Member Author

@bors-servo: r=kmc,munksgaard 369a568

@Manishearth
Copy link
Member Author

@bors-servo: r-

@Manishearth
Copy link
Member Author

@bors-servo: r=kmc,munksgaard 369a568

@Manishearth Manishearth reopened this Apr 27, 2015
@Manishearth
Copy link
Member Author

@bors-servo: r=kmc,munksgaard 369a568

@Manishearth
Copy link
Member Author

ah, works now.

@jdm jdm added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Apr 27, 2015
@bors-servo
Copy link
Contributor

⌛ Testing commit 369a568 with merge d7987e4...

bors-servo pushed a commit that referenced this pull request Apr 28, 2015
fixes #5724, #5737


uses https://github.com/Manishearth/rust-tenacious (can be moved in-tree if needed)

I can make it `Deny` by default too (I'll add a cargo feature to tenacious), though we might want it on
`Warn` until we get some mileage on it.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/5855)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

☀️ Test successful - android, gonk, linux1, linux2, mac1, mac2

@bors-servo bors-servo merged commit 369a568 into servo:master Apr 28, 2015
@Manishearth Manishearth deleted the nomove branch April 28, 2015 07:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Write a lint to flag moving Root values
7 participants