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

Improve generic usage of Pixel trait #996

Merged
merged 2 commits into from Feb 14, 2019

Conversation

Projects
None yet
4 participants
@rom1v
Copy link
Collaborator

commented Feb 13, 2019

In order to use either u8 or u16 for plane components, a Pixel trait regroups several "capabilities" that a generic pixel type must support, through trait inheritance.

That way, a generic function can just use the Pixel type, without listing them all:

fn f<T: Pixel>(...) { ... }

However, trait inheritance cannot express constraints about T for other types (like i32: AsPrimitive<T>). As a consequence, callers had to specify these additional constraints manually:

where
    T: Pixel,
    i32: AsPrimitive<T>,
    u32: AsPrimitive<T>,

This is redundant, because if T is a Pixel, then i32 and u32 necessarily implement AsPrimitive<T> (since Pixel is only implemented for u8 and u16).

To express these relationships, create a new trait, CastFromPrimitive<T>, representing the inverse of AsPrimitive<T>.

This paves the way to make Plane (and many other structures) generic over the pixel type conveniently.


Since Pixel is only implemented for u8 and u16 types, it is always possible to cast a Pixel to an integral primitive type. However, the compiler could not know this, because in theory any other type could also implement the Pixel trait.

To enforce this restriction, make the (main) integral primitive types implement CastForPrimitive<T> for any T which implements Pixel. That way, the compiler knows that a Pixel (whatever its concrete type) can always be cast to an integer.

Then, rewrite the constraints on convert_slice_2d() to use CastFromPrimitive, so that callers need not to add redundant constraints if T implements Pixel.

@tdaede

This comment has been minimized.

Copy link
Collaborator

commented Feb 13, 2019

Ah neat, when I originally wrote this I wasn't aware of CastFromPrimitive. This makes more sense.

@rom1v

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 13, 2019

@tdaede

I wasn't aware of CastFromPrimitive

I just defined it in this PR.

(it could be better to implement it upstream, in the num crate).

@tdaede

This comment has been minimized.

Copy link
Collaborator

commented Feb 13, 2019

Aha, didn't finish scrolling :)

@tdaede

This comment has been minimized.

Copy link
Collaborator

commented Feb 13, 2019

Looks like the changes need to be made to some of the tests as well.

@rom1v rom1v force-pushed the rom1v:castfrom branch from d346e20 to f1bc7ca Feb 14, 2019

@coveralls

This comment has been minimized.

Copy link

commented Feb 14, 2019

Coverage Status

Coverage decreased (-0.005%) to 79.269% when pulling caabe30 on rom1v:castfrom into 82a5270 on xiph:master.

@lu-zero
Copy link
Collaborator

left a comment

Might be noted that it is similar to the yet-to-stabilize try_from()

@rom1v

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 14, 2019

@lu-zero

Might be noted that it is similar to the yet-to-stabilize try_from()
Yes, but try_from() returns a Result<usize, TryFromIntError>.

Here, in non-generic code, we just use v as u16 or v as u8, without additional checks, so I guess we want the same behavior in generic code.

@rom1v rom1v force-pushed the rom1v:castfrom branch from f1bc7ca to 8f7c43f Feb 14, 2019

rom1v added some commits Feb 13, 2019

Improve generic usage of Pixel trait
In order to use either u8 or u16 for plane components, a Pixel trait
regroups several "capabilities" that a generic pixel type must support,
through trait inheritance.

That way, a generic function can just use the Pixel type, without
listing them all:

    fn f<T: Pixel>(...) { ... }

However, trait inheritance cannot express constraints about T for other
types (like "i32: AsPrimitive<T>"). As a consequence, callers had to
specify these additional constraints manually:

    where
        T: Pixel,
        i32: AsPrimitive<T>,
        u32: AsPrimitive<T>,

This is redundant, because if T is a Pixel, then i32 and u32 necessarily
implement AsPrimitive<T> (since Pixel is only implemented for u8 and
u16).

To express these relationships, create a new trait,
CastFromPrimitive<T>, representing the inverse of AsPrimitive<T>.

This paves the way to make Plane (and many other structures) generic
over the pixel type conveniently.
Adapt Pixel trait for convert_slice_2d()
Since Pixel is only implemented for u8 and u16 types, it is always
possible to cast a Pixel to an integral primitive type. However, the
compiler could not know this, because in theory any other type could
also implement the Pixel trait.

To enforce this restriction, make the (main) integral primitive types
implement CastForPrimitive<T> for any T which implements Pixel. That
way, the compiler knows that a Pixel (whatever its concrete type) can
always be cast to an integer.

Then, rewrite the constraints on convert_slice_2d() to use
CastFromPrimitive, so that callers need not to add redundant constraints
if T implements Pixel.

@rom1v rom1v force-pushed the rom1v:castfrom branch from 8f7c43f to caabe30 Feb 14, 2019

@tdaede

This comment has been minimized.

Copy link
Collaborator

commented Feb 14, 2019

Thanks, quickly merging before we break it again :)

@tdaede tdaede merged commit a99da11 into xiph:master Feb 14, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.