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

Remove scalar bound from geometry type defs #932

Merged
merged 2 commits into from Jul 10, 2021
Merged

Conversation

CAD97
Copy link
Contributor

@CAD97 CAD97 commented Jul 6, 2021

This was inconsistently applied, with some types having , some having
<T: Scalar>, and some having <T: RealField>.
This unifies all types to match the convention of Matrix:
Just declare at type def time, and apply bounds on impls only.

A significant advantage of this approach is const fn construction. Const
fn generics currently still can't have trait bounds, so any generic
const fn needs to only move opaque types around. Construction methods
such as new_unchecked or from_parts can be made const by removing their
generic bounds after this PR.

Actual constification is left to a follow-up PR.

Note that na::Transform is not loosened here, as it has more complicated
definition requirements.

This was inconsistently applied, with some types having <T>, some having
<T: Scalar>, and some having <T: RealField>.
This unifies all types to match the convention of Matrix:
Just declare <T> at type def time, and apply bounds on impls only.

A significant advantage of this approach is const fn construction. Const
fn generics currently still can't have trait bounds, so any generic
const fn needs to only move opaque types around. Construction methods
such as new_unchecked or from_parts can be made const by removing their
generic bounds after this PR.

Actual constification is left to a follow-up PR.

Note that na::Transform is _not_ loosened here, as it has more complicated
definition requirements.
@sebcrozet
Copy link
Member

Thank you for this PR! This is a good idea.

I think the CI is failing when nalgebra is built with the serde feature enabled. We probably need to add to the serde macro all the trait-bounds your removed from the type definition.

@CAD97
Copy link
Contributor Author

CAD97 commented Jul 9, 2021

That's what I get for not checking --all-features 😅

Should work now.

@CAD97
Copy link
Contributor Author

CAD97 commented Jul 10, 2021

I don't think that proptest failure is even possibly caused by my changes.

@sebcrozet
Copy link
Member

Yeah, this happens from time to time if we are unlucky with the test inputs (and get a particularly badly conditioned matrix).
I have re-run the tests.

@sebcrozet
Copy link
Member

It succeeds now, thanks!

@sebcrozet sebcrozet merged commit ac61e11 into dimforge:dev Jul 10, 2021
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.

None yet

2 participants