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

Support converting arrays of integers to Rect via TryFrom #1403

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

lolbinarycat
Copy link

No description provided.

lolbinarycat pushed a commit to lolbinarycat/rust-sdl2 that referenced this pull request Jun 7, 2024
Done as a seperate commit since it needs to reference the PR number.
@Cobrand
Copy link
Member

Cobrand commented Jun 11, 2024

Something tells me this will fail fmt

binarycat added 2 commits June 11, 2024 14:56
Done as a seperate commit since it needs to reference the PR number.
@antonilol
Copy link
Contributor

You should put the doc comments on try_from, not on the impl block (like the standard lib), this way compiler frontends like rust-analyzer can show the doc on hover.

Accepting all types that are TryInto<i32> (and the same for u32) will give inconsistent behavior on overflowing i32::MAX/2, because when it is out of bounds for i32, it will error, but when it is only out of bounds for the rect, it will clamp (existing behavior).

Also, using an array does not make sense to me because a Rect is not some sort of list, each value has a distinct meaning (namely x, y, width and height).

@lolbinarycat
Copy link
Author

Accepting all types that are TryInto (and the same for u32) will give inconsistent behavior on overflowing i32::MAX/2, because when it is out of bounds for i32, it will error, but when it is only out of bounds for the rect, it will clamp (existing behavior).

do you have a better idea? i don't think rust provides a generic way to do a clamping type conversion.

Also, using an array does not make sense to me because a Rect is not some sort of list, each value has a distinct meaning (namely x, y, width and height).

yet it still supports casts from tuples, which are basically just heterogeneous lists.

@antonilol
Copy link
Contributor

do you have a better idea? i don't think rust provides a generic way to do a clamping type conversion.

The standard library does not provide this, but you can make a custom trait to do saturating conversion to i32 and u32. (To me this does seem overkill for one conversion function.)

yet it still supports casts from tuples, which are basically just heterogeneous lists.

A tuple gets closer to Rect than an array (type per field), but fields are still unnamed.

@Cobrand
Copy link
Member

Cobrand commented Jun 13, 2024

I do agree that tuples are better, but maybe there is a specific usecase like matrix computing or something. @lolbinarycat why do you need this specifically?

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.

3 participants