Skip to content

Conversation

@Firestar99
Copy link
Member

This makes the traits VectorOrScalar, Vector, Scalar and image::params::SampleType implicitly be Send + Sync + 'static. This has always been true for all impl anyway, and as these are all sealed traits, this change is not breaking.

This mostly helps me with my bindless API where I always have to do Generic: SampleType + Send + Sync + 'static and now I can just do Generic: SampleType.

/// Implementing this trait on non-scalar or non-vector types may break assumptions about other
/// unsafe code, and should not be done.
pub unsafe trait VectorOrScalar: Default {
pub unsafe trait VectorOrScalar: Default + Send + Sync + 'static {
Copy link
Member

@eddyb eddyb Dec 20, 2024

Choose a reason for hiding this comment

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

Would it make sense to also throw in Copy? (I can't think of a lot of other traits, maybe PartialEq but ehhh).

Oh, actually:

pub unsafe trait Scalar:
VectorOrScalar<Scalar = Self> + Copy + Default + crate::sealed::Sealed

So that confirms my expectation that Copy should be included. Can you also add Send + Sync + 'static to that trait?

Also, why is this trait not actually sealed the same way the Scalar trait is? Maybe leftover from the glam days? (unless it was sealed and that got undone)

Copy link
Member

@eddyb eddyb Dec 20, 2024

Choose a reason for hiding this comment

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

Oops I missed the VectorOrScalar<Scalar = Self> that trait Scalar requires.

In that case, can you just move Copy from Scalar to VectorOrScalar?

Copy link
Member Author

@Firestar99 Firestar99 Dec 20, 2024

Choose a reason for hiding this comment

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

I added that trait so likely I just forgot Actually trait Vector is not sealed, only Scalar is

@Firestar99 Firestar99 added this pull request to the merge queue Dec 20, 2024
Merged via the queue into main with commit 565db88 Dec 20, 2024
7 checks passed
@Firestar99 Firestar99 deleted the vector_scalar_imply branch December 20, 2024 15:17
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