Skip to content

make vector and scalar traits imply Send + Sync + 'static #187

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.

Sign up for GitHub

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

Merged
Firestar99 merged 2 commits into from
Dec 20, 2024
Merged

make vector and scalar traits imply Send + Sync + 'static #187

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8788f92
make vector and scalar traits imply `Send + Sync + 'static`
Firestar99 Dec 20, 2024
14c03e5
make vector and scalar traits also imply `Copy`
Firestar99 Dec 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion crates/spirv-std/src/vector.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use glam::{Vec3Swizzles, Vec4Swizzles};
/// # Safety
/// 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 •
edited
Loading

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:

rust-gpu/crates/spirv-std/src/scalar.rs

Lines 11 to 12 in 8788f92

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 •
edited
Loading

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 •
edited
Loading

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

/// Either the scalar component type of the vector or the scalar itself.
type Scalar: Scalar;

Expand Down
Loading