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

Move comparisons to traits #265

Merged
merged 3 commits into from
Apr 4, 2022
Merged

Move comparisons to traits #265

merged 3 commits into from
Apr 4, 2022

Conversation

calebzulawski
Copy link
Member

@calebzulawski calebzulawski commented Mar 13, 2022

A simpler variant of #206.

  • Comparisons are moved to SimdPartialEq, SimdPartialOrd, and SimdOrd. The function names are prefixed with simd_ to disambiguate from the regular PartialEq etc functions. With the functions on traits instead of Simd directly, shadowing the function names doesn't work very well.
  • Floating point Ord-like functions are put into a SimdFloat trait. The intention is that eventually (some time after this PR) all floating point functions will be moved from Simd to SimdFloat, and the same goes for future SimdInt/SimdUint traits.

crates/core_simd/src/ord.rs Outdated Show resolved Hide resolved
crates/core_simd/src/ord.rs Show resolved Hide resolved
@calebzulawski calebzulawski requested review from programmerjake and workingjubilee and removed request for workingjubilee March 23, 2022 21:14
@workingjubilee
Copy link
Member

Apologies, I am not trying to ignore this, I just was getting sidetracked by an Adventure or two.

Copy link
Member

@programmerjake programmerjake left a comment

Choose a reason for hiding this comment

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

lgtm

crates/core_simd/src/ord.rs Show resolved Hide resolved
@programmerjake
Copy link
Member

we should post somewhere users will notice that this breaks API compatibility.

@workingjubilee
Copy link
Member

workingjubilee commented Apr 4, 2022

Okay. Having reviewed this properly, I feel like the ergonomics of these ops could probably be improved further and I think we will eventually find we want to encode the idea of associated masking types in a separate trait, as it feels plausible masked ops will want to not have to depend on SimdPartialEq being defined, but having said that I see no reason not to merge this as it stands as it is a significant improvement.

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