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 &Root<T> checker #11190

Merged
merged 1 commit into from Jun 5, 2016
Merged

add &Root<T> checker #11190

merged 1 commit into from Jun 5, 2016

Conversation

@mrmiywj
Copy link
Contributor

mrmiywj commented May 15, 2016

Thank you for contributing to Servo! Please replace each [ ] by [X] when the step is complete, and replace __ with appropriate data:

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #11137 (github issue number if applicable).

Either:

  • There are tests for these changes OR
  • These changes do not require tests because _____

Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process.


This change is Reviewable

@highfive
Copy link

highfive commented May 15, 2016

Heads up! This PR modifies the following files:

@wafflespeanut
Copy link
Member

wafflespeanut commented May 15, 2016

We also need to update the tests in servo_tidy_tests, so that we can make sure that we're getting the warning.

@highfive
Copy link

highfive commented May 15, 2016

New code was committed to pull request.

fn test_fun3(x:&Root<isize>) {
()
}

This comment has been minimized.

@wafflespeanut

wafflespeanut May 15, 2016

Member

Well, this won't be needing a new function, but well, it's just a test... 😄

@wafflespeanut
Copy link
Member

wafflespeanut commented May 15, 2016

Note that the raised warning should be verified in test_tidy.py. Also, ./mach test-tidy will show the files which make use of &Root<T>. We should replace them before we land.

@mrmiywj
Copy link
Contributor Author

mrmiywj commented May 15, 2016

Does it mean I need to replace them by hands? It's must be a great amount of work..

@wafflespeanut
Copy link
Member

wafflespeanut commented May 15, 2016

Not if we use find and awk or sed to do the replacement. Moreover, the failed travis build shows that there are only 3 occurrences of the thing in our codebase :)

@mrmiywj
Copy link
Contributor Author

mrmiywj commented May 15, 2016

There maybe some problems if we change &Root to &T. It will cause type mismatch.
In this file mentioned in travis build log.

@highfive
Copy link

highfive commented May 15, 2016

New code was committed to pull request.

1 similar comment
@highfive
Copy link

highfive commented May 15, 2016

New code was committed to pull request.

@cbrewster
Copy link
Member

cbrewster commented May 15, 2016

Is there any reason why we have a from_rooted it seems like we could change all those to from_ref. Or even better, we could move that to Root<T> As the comment there suggests.

@jdm
Copy link
Member

jdm commented May 15, 2016

Hmm, unroot and the PartialEq implementation for Root<T> are a problem. I'm not sure what to do about those.

@nox
Copy link
Member

nox commented May 16, 2016

IMO RootCollection::unroot should be inlined directly in Root::<T>::drop because you shouldn't be able to unroot something without losing the Root itself (even more so because unroot isn't even unsafe).

For PartialEq, we could maybe remove the impl and see how many comparisons now need an additional bunch of &* magic.

@KiChjang
Copy link
Member

KiChjang commented May 31, 2016

What's the status of this?

@mrmiywj
Copy link
Contributor Author

mrmiywj commented May 31, 2016

@KiChjang
Hi, Sorry for that I was busy with my finals.
I will go on this in this week

@jdm
Copy link
Member

jdm commented May 31, 2016

The other status is that we need to figure out how to avoid a couple valid uses of : Root<T> that exist in the codebase, otherwise this can't merge.

@jdm
Copy link
Member

jdm commented May 31, 2016

#11522 should address all the problem instances, so this should be ready to merge as soon as that PR does!

update rust_tidy tests

update rust_tidy.rs and test_tidy.py
@highfive
Copy link

highfive commented Jun 5, 2016

New code was committed to pull request.

@jdm
Copy link
Member

jdm commented Jun 5, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Jun 5, 2016

📌 Commit a17ae72 has been approved by jdm

@highfive highfive assigned jdm and unassigned wafflespeanut Jun 5, 2016
@bors-servo
Copy link
Contributor

bors-servo commented Jun 5, 2016

Testing commit a17ae72 with merge 93f137f...

bors-servo added a commit that referenced this pull request Jun 5, 2016
add &Root<T> checker

Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data:
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #11137  (github issue number if applicable).

Either:
- [X] There are tests for these changes OR
- [ ] These changes do not require tests because _____

Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11190)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jun 5, 2016

💔 Test failed - linux-rel

@highfive
Copy link

highfive commented Jun 5, 2016

  ▶ FAIL [expected PASS] /css-transforms-1_dev/html/transform-table-007.htm
  └   → /css-transforms-1_dev/html/transform-table-007.htm a5c014b20ef1363bea6f24eda28c7efb7c45698a
/css-transforms-1_dev/html/reference/transform-blank-ref.htm fa6407b1acbbfea27e27061e7d1bdeca98e4a728
Testing a5c014b20ef1363bea6f24eda28c7efb7c45698a == fa6407b1acbbfea27e27061e7d1bdeca98e4a728
@wafflespeanut
Copy link
Member

wafflespeanut commented Jun 5, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Jun 5, 2016

Previous build results for android, arm32, arm64, linux-dev, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows are reusable. Rebuilding only linux-rel...

@bors-servo
Copy link
Contributor

bors-servo commented Jun 5, 2016

@bors-servo bors-servo merged commit a17ae72 into servo:master Jun 5, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
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.

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