Various refactorings #411

Mingun · 2020-03-15T14:57:01Z

Part of large changeset, which includes also #410. Aimed to improve ergonomics, fill gaps in the API and overall consistency.

nical

There are some welcome additions in here like the extra operators, PartialOrd for min/max and removing the try! macro, which could land without semver breaking changes if you want to separate them in a separate PR.

src/point.rs


        `try!` was deprecated, use `?`


        Fix misprints


        Convert ordinal comment to doc-comment


        Remove unnecessary PartialEq bound


        Use approxord for compare Points/Sizes/Vectors


        Add `#[inline]` to all no-op conversion routines


        Use ordinal naming conversion for types:

- Full name `Unit` in struct definition, because that look better in the documentation - Short name `U` in implementation blocks, which reduces noise in documentation


        Add some doctests


        Point: merge Zero impl block with main impl block. That creates more …

…beatify looking documentation


        Point: move lerp() to main impl block


        Point: merge `to_` converter methods into main impl block


        Point: move impl block for PartialOrd out from between operators impl…

… blocks


        Point: move impl blocks of Points itself to other such impl blocks


        Point: reorder operator tests


        Point: move operator tests to submodule `ops`


        Point: add tests for existing operators


        Point: more complete support for arithmetic operations


        Point: reorder operator implementations (split for several commits fo…

…r better diffs - 1)


        Point: reorder operator implementations (split for several commits fo…

…r better diffs - 2)

Mingun · 2020-03-22T19:19:23Z

As the number of commits approaches an obscene large number, I decided to finish part of them in this PR. All but the last are completely backward compatible. The last changes the type requirements for the *Assign operators and technically is a breaking change. However, it is highly unlikely that it will break anything in user code, because I'm bet that Point types always instantiated with types where the implementation of ordinary and *Assign operators is the same.

This does not mean that I propose to make this change in the patch, but this change can raise the version to 0.21 without the danger of actually breaking the user code. Although, if you don't want to take risks, I can exclude it from this PR and include it in one of my next PR with such breaking changes.

Summary of changes:

Replace deprecated try!
Mostly unified usage of #[inline]
Implement min/max/clamp in terms of PartialOrd for all types
Increase test coverage for operators of Points
Implement *Assign counterparts for operators of Points
Implement Round/Floor/Ceil/Zero for Points
Add many examples as doctests to various methods

nical · 2020-03-24T19:04:15Z

Sorry for the delay. Please remove the *Assign operators for now. The change is not particularly important so even if it isn't likely to break much code, let's just bundle it with the other breaking changes since there are going to be some. The rest looks good at a glance.

Mingun · 2020-03-24T19:17:09Z

Done

nical · 2020-03-25T09:45:03Z

@Mingun thanks, there are a few build errors which should be straightforward to fix.


        Point: new operators

- Implement Point2D - Size2D operator - Implement Point3D +- Size3D operators - Implement -Point3D operator - Implement *Assign counterparts for operators


        Point: implement Round/Floor/Ceil for Points


        Point: implement Zero


        Point: replace usage add_size() with operator+

Mingun · 2020-03-25T14:37:14Z

The costs of frequent rebases, although I thought I tested all commits for buildable :/. Now all commits buildable

nical · 2020-03-25T16:14:02Z

Thank you for your patience.

@bors-servo r+

bors-servo · 2020-03-25T16:14:03Z

📌 Commit 444db0c has been approved by nical

bors-servo · 2020-03-25T16:14:07Z

⌛ Testing commit 444db0c with merge 190adc0...

bors-servo · 2020-03-25T16:23:56Z

☀️ Test successful - checks-travis
Approved by: nical
Pushing 190adc0 to master...


        Auto merge of #412 - Mingun:size, r=nical

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.


        Auto merge of #423 - Mingun:translation, r=nical

Some refactoring, *Assign operators and box transform methods for Translations As a continuation of [our discourse](#411 (comment)) about by value vs by reference: added translation methods for Boxes takes its by reference, but as [godbolt](https://rust.godbolt.org/z/Kgqdjd) shows, even without forced `#[inline]` directive compiler smart enough to inline such small function and the code the same in both cases. Therefore, I'm still in favour of taking everything by value in methods that almost only change unit. For now I'm, however, use reference as in surrounding code.

Mingun mentioned this pull request Mar 15, 2020

Some refactoring for size.rs #412

Merged

Mingun force-pushed the Mingun:point branch from da04e16 to f958586 Mar 15, 2020

nical requested changes Mar 20, 2020

View changes

src/point.rs Show resolved Hide resolved

src/point.rs Outdated Show resolved Hide resolved

src/point.rs Show resolved Hide resolved

src/point.rs Outdated Show resolved Hide resolved

Mingun force-pushed the Mingun:point branch from f958586 to ee69dd4 Mar 21, 2020

Mingun requested a review from nical Mar 21, 2020

Mingun added 19 commits Mar 15, 2020

`try!` was deprecated, use `?`

610eb43

Fix misprints

1632066

Convert ordinal comment to doc-comment

6d6276a

Remove unnecessary PartialEq bound

20f9211

Use approxord for compare Points/Sizes/Vectors

74ad7d0

Add `#[inline]` to all no-op conversion routines

b76eea3

Use ordinal naming conversion for types:

205a701

- Full name `Unit` in struct definition, because that look better in the documentation - Short name `U` in implementation blocks, which reduces noise in documentation

Add some doctests

6d7f30d

Point: merge Zero impl block with main impl block. That creates more …

333af39

…beatify looking documentation

Point: move lerp() to main impl block

21badb3

Point: merge `to_` converter methods into main impl block

5610434

Point: move impl block for PartialOrd out from between operators impl…

df3948d

… blocks

Point: move impl blocks of Points itself to other such impl blocks

c8aab1e

Point: reorder operator tests

769519f

Point: move operator tests to submodule `ops`

45e598b

Point: add tests for existing operators

e3331d1

Point: more complete support for arithmetic operations

52c70bf

Point: reorder operator implementations (split for several commits fo…

ee74b05

…r better diffs - 1)

Point: reorder operator implementations (split for several commits fo…

7e82102

…r better diffs - 2)

Mingun force-pushed the Mingun:point branch from ee69dd4 to eb8452f Mar 22, 2020

nical changed the title ~~Some refactoring for point.rs~~ Various refactorings Mar 24, 2020

Mingun force-pushed the Mingun:point branch from eb8452f to 19c0e97 Mar 24, 2020

Mingun added 4 commits Mar 15, 2020

Point: new operators

aca2e08

- Implement Point2D - Size2D operator - Implement Point3D +- Size3D operators - Implement -Point3D operator - Implement *Assign counterparts for operators

Point: implement Round/Floor/Ceil for Points

67585c4

Point: implement Zero

2b8c735

Point: replace usage add_size() with operator+

Loading status checks…

444db0c

Mingun force-pushed the Mingun:point branch from 19c0e97 to 444db0c Mar 25, 2020

bors-servo merged commit 190adc0 into servo:master Mar 25, 2020
2 checks passed

2 checks passed

Travis CI - Pull Request Build Passed
Details

homu Test successful
Details

This was referenced Mar 25, 2020

Provide `approx_eq` and implement `ApproxEq` for Transform2D and Transform3D #409

Merged

Some refactoring and documentation for scale.rs #410

Merged

Mingun deleted the Mingun:point branch Mar 25, 2020

Mingun mentioned this pull request Apr 3, 2020

Some refactoring, *Assign operators and box transform methods for Translations #423

Merged

servo / euclid

Various refactorings #411

Various refactorings #411

Mingun commented Mar 15, 2020

nical left a comment

Mingun commented Mar 22, 2020

nical commented Mar 24, 2020

Mingun commented Mar 24, 2020

nical commented Mar 25, 2020

Mingun commented Mar 25, 2020

nical commented Mar 25, 2020

bors-servo commented Mar 25, 2020

bors-servo commented Mar 25, 2020

bors-servo commented Mar 25, 2020

servo / euclid

Join GitHub today

Various refactorings #411

Various refactorings #411

Conversation

Mingun commented Mar 15, 2020

nical left a comment

Mingun commented Mar 22, 2020

nical commented Mar 24, 2020

Mingun commented Mar 24, 2020

nical commented Mar 25, 2020

Mingun commented Mar 25, 2020

nical commented Mar 25, 2020

bors-servo commented Mar 25, 2020

bors-servo commented Mar 25, 2020

bors-servo commented Mar 25, 2020