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

Go through style::hash for the TransitionProperty set #18755

Closed
wants to merge 1 commit into from

Conversation

bholley
Copy link
Contributor

@bholley bholley commented Oct 5, 2017

In #18604, emilio switched us from hash::HashSet to std::collections::HashSet for the TransitionProperty set. In turn, this means that Manishearth's change in #18712 didn't catch this use of DefaultHasher. This means that we still have usage of RandomState in stylo, which means that we'll still have crashes on the systems where RandomState breaks.

Mentions moved to a comment to avoid email spam down the road.


This change is Reviewable

@highfive
Copy link

highfive commented Oct 5, 2017

Heads up! This PR modifies the following files:

@highfive
Copy link

highfive commented Oct 5, 2017

warning Warning warning

  • These commits modify style code, but no tests are modified. Please consider adding a test!

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Oct 5, 2017
@bholley
Copy link
Contributor Author

bholley commented Oct 5, 2017

Also note that I fixed this in the beta uplift in [1], which is actually how I noticed it, since #18604 is not on beta.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1385971

@bholley
Copy link
Contributor Author

bholley commented Oct 5, 2017

CC various other style/ people to be aware that we should avoid any usage of DefaultHasher and that all usage should go through style::hash @SimonSapin @nox @upsuper @hiikezoe @birtles @heycam @Manishearth @emilio

@upsuper
Copy link
Contributor

upsuper commented Oct 5, 2017

@bors-servo r+

You should probably remove the @ to manish and emilio as well.

@bors-servo
Copy link
Contributor

📌 Commit db6bfe9 has been approved by upsuper

@highfive highfive assigned upsuper and unassigned wafflespeanut Oct 5, 2017
@highfive highfive 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 Oct 5, 2017
@bholley
Copy link
Contributor Author

bholley commented Oct 5, 2017

@bors-servo p=1

@upsuper
Copy link
Contributor

upsuper commented Oct 5, 2017

We still have several usage of std::collections for build script and custom derive code. I guess those are fine, since they are not built into the final code.

@emilio
Copy link
Member

emilio commented Oct 5, 2017

@bors-servo r-

@emilio
Copy link
Member

emilio commented Oct 5, 2017

That fix is #18763

@hiikezoe
Copy link
Contributor

hiikezoe commented Oct 5, 2017

Just to clarify, both this and #18763 don't need to be uplifted to 57, right?

@hiikezoe
Copy link
Contributor

hiikezoe commented Oct 5, 2017

Oops, I missed Bobby's comment, this has been already uplifted to 57.

@bors-servo
Copy link
Contributor

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

@highfive highfive added S-needs-rebase There are merge conflict errors. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Oct 6, 2017
@emilio
Copy link
Member

emilio commented Oct 6, 2017

That hash is no more.

@emilio emilio closed this Oct 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-needs-rebase There are merge conflict errors.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants