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.

Sign up for GitHub

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

Jump to bottom

Use boolean instead of float to avoid nightly warning #16897

Closed
wants to merge 1 commit into from
Closed

Use boolean instead of float to avoid nightly warning #16897

wants to merge 1 commit into from

Conversation

Manishearth
Copy link
Member

@Manishearth Manishearth commented May 16, 2017 •
edited by larsbergstrom

It's just nicer to match on a bool, too

This change is Reviewable

@highfive
Copy link

Heads up! This PR modifies the following files:

@highfive
Copy link

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label May 16, 2017
@nox
Copy link
Contributor

nox commented May 16, 2017

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit 6e42d5a has been approved by nox

@highfive highfive assigned nox and unassigned cbrewster May 16, 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 May 16, 2017
@jdm
Copy link
Member

jdm commented May 16, 2017

@bors-servo: r-

Checking files for tidiness...

./ports/geckolib/glue.rs:2207: found an empty line following a {

@jdm jdm added S-fails-tidy `./mach test-tidy` reported errors. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels May 16, 2017
emilio
emilio reviewed May 17, 2017
View reviewed changes
ports/geckolib/glue.rs Outdated
@@ -2216,14 +2214,14 @@ fn fill_in_missing_keyframe_values(all_properties: &[TransitionProperty],
return;
}

let keyframe = match offset {
0. => unsafe {
let keyframe = if is_initial {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not keeping the offset parameter and just do if offset == 0. here? I think boolean arguments hard to read.

Copy link
Contributor

@nox nox May 17, 2017 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@emilio File a follow-up to use an enum. Having a float where only two values matter is even worse, and should never have been r+'d in the first place.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I had not noticed that this didn't land yet, so maybe we should just require from Manish to do that damn enum now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@emilio never, never, never compare float values for equality if you can help it.

And unreachable branches are not great.

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label May 17, 2017
@wafflespeanut
Copy link
Contributor

@bors-servo r=nox

@bors-servo
Copy link
Contributor

📌 Commit c3660dc has been approved by nox

@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 May 18, 2017
@bors-servo
Copy link
Contributor

⌛ Testing commit c3660dc with merge 60a563a...

bors-servo pushed a commit that referenced this pull request May 18, 2017
Auto merge of #16897 - Manishearth:bool, r=nox
60a563a 
Use boolean instead of float to avoid nightly warning

It's just nicer to match on a bool, too

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

@Manishearth Tidy still fails with the empty line.

@wafflespeanut
Copy link
Contributor

@bors-servo r-

@wafflespeanut wafflespeanut reopened this May 18, 2017
@nox
Copy link
Contributor

nox commented May 18, 2017

@bors-servo r- force

@nox
Copy link
Contributor

nox commented May 18, 2017

This makes Homu very unhappy, and @Manishearth didn't check the box that let other people edit their PRs:

± git push -f https://github.com/Manishearth/servo.git HEAD:bool
Décompte des objets: 515, fait.
Delta compression using up to 8 threads.
Compression des objets: 100% (250/250), fait.
Écriture des objets: 100% (515/515), 122.05 KiB | 0 bytes/s, fait.
Total 515 (delta 420), reused 336 (delta 258)
remote: Resolving deltas: 100% (420/420), completed with 175 local objects.
To https://github.com/Manishearth/servo.git
 ! [remote rejected]       HEAD -> bool (permission denied)
error: impossible de pousser des références vers 'https://github.com/Manishearth/servo.git'

@nox nox closed this May 18, 2017
@nox nox reopened this May 18, 2017
@nox nox closed this May 18, 2017
@nox
Copy link
Contributor

nox commented May 18, 2017

Letting it closed because Homu still says it's pending if it stays open.

@Manishearth Manishearth mentioned this pull request May 18, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nox nox nox left review comments

@emilio emilio emilio left review comments

Assignees

@nox nox

Labels
S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. S-fails-tidy `./mach test-tidy` reported errors.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@Manishearth @highfive @nox @bors-servo @jdm @wafflespeanut @SimonSapin @emilio @cbrewster