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

Feature/ordered float #399

Merged
merged 12 commits into from
Oct 22, 2024
Merged

Conversation

bassmanitram
Copy link

@bassmanitram bassmanitram commented Oct 22, 2024

This PR addresses issue #398 by implementing a feature named ordered-float that activates the same-named feature in value-trait which itself activates the wrapping of floats with OrderedFloat and thus allows StaticValue to be std::cmp::Eq. Various feature-related "tweaks" are also implemented, while trying to use as few #cfg[feature...] annotations as possible.

Obvious the PR is dependent upon the value-trait PR, but while that is pending Cargo.toml in this PR's source branch patches the value-trait crate source to be the source repo and branch of the value-trait PR.

Martin Bartlett added 5 commits October 21, 2024 17:24
This includes adding a couple of unsafe blocks that, for _some_ reason are not required (or, at least not FLAGGED) when
my new feature is activated... I'll look into that later. For now, though, stuff appears to build correctly
@bassmanitram
Copy link
Author

bassmanitram commented Oct 22, 2024

This is a draft PR at the moment. However, at the time of this comment all existing tests pass with and without the new feature, and all my clippy lints have been addressed.

@Licenser
Copy link
Member

value-trait is updated to .10.1 with your ordered-float

@Licenser
Copy link
Member

can we add the feature to the test matrix so we have the ordered float path covered?

Martin Bartlett added 2 commits October 22, 2024 11:55
All tests pass with and without - as an intellectual exercise I'm looking at LazyValue too :)
@bassmanitram
Copy link
Author

can we add the feature to the test matrix so we have the ordered float path covered?

Your wish is my command (which I was actually doing when you posted that :) )

@Licenser
Copy link
Member

great minds think alike :)

Copy link
Member

@Licenser Licenser left a comment

Choose a reason for hiding this comment

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

Awesome improvement!

@Licenser Licenser linked an issue Oct 22, 2024 that may be closed by this pull request
@bassmanitram
Copy link
Author

bassmanitram commented Oct 22, 2024

The goal of solving the Eq issue in my main project has been achieved - still a couple of issues that will likely be best dealt with in that project rather than hear (the necessary unsafe and mutability differences and a couple of missing methods only used in tests)

Once I get those tweaked away, I'll see if my full "droppin" goal is achieved.

@bassmanitram bassmanitram marked this pull request as ready for review October 22, 2024 12:01
@Licenser
Copy link
Member

Awesome, I think this needs a cargo fmt to make the CI happy; otherwise good to merge :) if I can help with some thoughts on other problems feel free to ping

@Licenser Licenser merged commit de7b6c3 into simd-lite:main Oct 22, 2024
23 checks passed
@bassmanitram bassmanitram deleted the feature/ordered-float branch October 28, 2024 16:31
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.

owned::Value and borrowed::Value don't implement std::cmp::Eq
2 participants