Skip to content

Comments

Use more Into<Point> instead of Point for arguments#466

Merged
sagudev merged 1 commit intolinebender:mainfrom
sagudev:more_into
Jul 17, 2025
Merged

Use more Into<Point> instead of Point for arguments#466
sagudev merged 1 commit intolinebender:mainfrom
sagudev:more_into

Conversation

@sagudev
Copy link
Contributor

@sagudev sagudev commented Jul 16, 2025

We already did for most of Point arguments. This is especially relevant for users of euclid now that we have #463.

Signed-off-by: sagudev <16504129+sagudev@users.noreply.github.com>
@sagudev sagudev changed the title Use Into<T> insted of T for arguments Use more Into<T> insted of T for arguments Jul 16, 2025
@sagudev
Copy link
Contributor Author

sagudev commented Jul 16, 2025

It would be nice to also do this on traits (Shape, paramcurves*), but that would be breaking change.

Copy link
Member

@tomcur tomcur left a comment

Choose a reason for hiding this comment

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

I'm cautiously in favor of doing this for Point, but we may want to make sure we only do this for types whose From impls are free (within the confines of this crate). Otherwise the API would hide conversions that could potentially needlessly slow down user code, if the same conversions are performed multiple times.

From<TranslateScale> for Affine is not quite free, and, e.g., for Rect (not included here), From<(Point, Size)> and From<euclid::Rect<f64, UnknownUnit>> aren't free.

@sagudev
Copy link
Contributor Author

sagudev commented Jul 16, 2025

I'm cautiously in favor of doing this for Point, but we may want to make sure we only do this for types whose From impls are free (within the confines of this crate). Otherwise the API would hide conversions that could potentially needlessly slow down user code, if the same conversions are performed multiple times.

From for Affine is not quite free, and, e.g., for Rect (not included here), From<(Point, Size)> and From<euclid::Rect<f64, UnknownUnit>> aren't free.

I agree. I forgot that From<TranslateScale> for Affine existed. Will drop last commit.

@sagudev sagudev changed the title Use more Into<T> insted of T for arguments Use more Into<Point> insted of Point for arguments Jul 16, 2025
@sagudev sagudev requested a review from tomcur July 16, 2025 09:46
@sagudev sagudev mentioned this pull request Jul 17, 2025
@sagudev sagudev changed the title Use more Into<Point> insted of Point for arguments Use more Into<Point> instead of Point for arguments Jul 17, 2025
Copy link
Member

@tomcur tomcur left a comment

Choose a reason for hiding this comment

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

Thanks!

@sagudev sagudev added this pull request to the merge queue Jul 17, 2025
Merged via the queue into linebender:main with commit 99a029f Jul 17, 2025
15 checks passed
@sagudev sagudev deleted the more_into branch July 17, 2025 11:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants