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

Some refactoring and documentation for scale.rs #410

Merged
merged 7 commits into from Mar 30, 2020
Merged

Conversation

@Mingun
Copy link
Contributor

Mingun commented Mar 14, 2020

Actually, this is part of a changes whose ultimate goal is to improve the ergonomics of the library. There are too many of them, so some of them I decided to provide in separate PRs. That is first.

Commit 19ddcfe introduce soft breaking change. I call it soft because very unlikely, that users faced with them, because that only replaces borrow with move for self parameter -- but if you don't use UFCS, you even will not notice that. And because before that change Copy type was required, after that change that method always called with Copy types, so no errors, related to borrow checker, can occur if you simply upgraded to new version of the library. Benefit in that after this change you can control if you wish to make a copy, and that methods can now work with non-Copy types.

@Mingun Mingun force-pushed the Mingun:scale branch from 58c7764 to 4802499 Mar 14, 2020
@Mingun Mingun changed the title Some refactoring of scale.rs Some refactoring and documentation for scale.rs Mar 15, 2020
This was referenced Mar 15, 2020
Copy link
Collaborator

nical left a comment

The review comments from #412 apply here too.

@@ -65,7 +65,7 @@ impl<T: Clone + One + Div<T, Output = T>, Src, Dst> Scale<T, Src, Dst> {
/// The inverse Scale (1.0 / self).
pub fn inv(&self) -> Scale<T, Dst, Src> {
let one: T = One::one();
Scale::new(one / self.get())
Scale::new(one / self.0.clone())

This comment has been minimized.

@nical

nical Mar 20, 2020

Collaborator

I find the previous version more readable.

This comment has been minimized.

@Mingun

Mingun Mar 20, 2020

Author Contributor

This is just preparation to change &self to self. get still require Clone bound (although at my sight, I would completely delete that method)

src/scale.rs Outdated Show resolved Hide resolved
@Mingun Mingun force-pushed the Mingun:scale branch from 2ce37e1 to 6a057c7 Mar 21, 2020
@Mingun Mingun requested a review from nical Mar 21, 2020
bors-servo added a commit that referenced this pull request Mar 25, 2020
Various refactorings

Part of large changeset, which includes also #410. Aimed to improve ergonomics, fill gaps in the API and overall consistency.
@bors-servo
Copy link
Contributor

bors-servo commented Mar 25, 2020

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

@Mingun Mingun force-pushed the Mingun:scale branch from 6a057c7 to 6e4547a Mar 28, 2020
bors-servo added a commit that referenced this pull request Mar 30, 2020
Some refactoring for size.rs

Part of large changeset, which includes also #410 and #411. Aimed to improve ergonomics, fill gaps in the API and overall consistency.
@nical
Copy link
Collaborator

nical commented Mar 30, 2020

@bors-servo
Copy link
Contributor

bors-servo commented Mar 30, 2020

📌 Commit 69a373e has been approved by nical

@bors-servo
Copy link
Contributor

bors-servo commented Mar 30, 2020

Testing commit 69a373e with merge 6dada46...

@bors-servo
Copy link
Contributor

bors-servo commented Mar 30, 2020

☀️ Test successful - checks-travis
Approved by: nical
Pushing 6dada46 to master...

@bors-servo bors-servo merged commit 6dada46 into servo:master Mar 30, 2020
2 checks passed
2 checks passed
Travis CI - Pull Request Build Passed
Details
homu Test successful
Details
@Mingun Mingun deleted the Mingun:scale branch Mar 30, 2020
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.

None yet

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