-
Notifications
You must be signed in to change notification settings - Fork 62
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
implemented palette angle/bool_mask/num traits for simba's Wide* wrapper types #431
base: master
Are you sure you want to change the base?
Conversation
they are implemented in the palette carte itself Ogeon/palette#431
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! I'm going to be a bit busy today, so I may wait with a proper review until later, but I skimmed through it and it looks good so far.
CodSpeed Performance ReportMerging #431 will not alter performanceComparing Summary
|
I don't think the last failing no_std test/check in CI is related to my changes error[E0557]: feature has been removed
--> no_std_test/src/main.rs:1:42
|
1 | #![cfg_attr(feature = "nightly", feature(start))]
| ^^^^^ feature has been removed
|
= note: not portable enough and never RFC'd
error: cannot find attribute `start` in this scope
--> no_std_test/src/main.rs:10:3
|
10 | #[start]
| ^^^^^
|
= note: `start` is in scope, but it is a function, not an attribute
error[E0[60](https://github.com/Ogeon/palette/actions/runs/13945384633/job/39033052288?pr=431#step:4:61)1]: `main` function not found in crate `no_std_test`
--> no_std_test/src/main.rs:30:13
|
30 | fn main() {}
| ^ consider adding a `main` function to `no_std_test/src/main.rs`
|
Yeah, it looks like they changed some things on nightly. 🤔 I can look into it when I'm free and let you know when it should work again, unless someone else gets to it first. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The no_std
test has been updated. You can rebase your branch to bring the change in. I have also eyed through the changes again and I think we just need to relax the features a bit to not pull in std
. Speaking of, it may be a good opportunity to extend the all_features
feature in the no_std_test
crate with palette/wide
and palette/simba
to verify the setup.
Also, Palette doesn't use the feat:
, etc. prefixes for anything, so you can drop them from the commits and PR. I'll ask you to squash everything at the end, so feel free to wait until then. 🙂
But thanks again for submitting this and also for adding the unit tests! 🎉
@@ -71,6 +72,10 @@ version = "0.7.3" | |||
optional = true | |||
default-features = false | |||
|
|||
[dependencies.simba] | |||
version = "0.9.0" | |||
optional = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's also disable default features so we don't impose more than we use. We may have to add opt-ins for std
and libm
with that, similar to wide
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review and suggestions. I rebased my branch, and implemented your suggestion regarding no_std_test
and prevented pulling in std
when not enabled via palette's std
feature.
Hope that is what you meant :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, thank you! I'm just wondering now if some of them can be skipped, if they aren't strictly necessary for the implementation here. 🤔 I looked around a bit and we can perhaps skip enabling most or all of them on wide
and simba
. Palette should only enable those that are necessary for compiling itself, after all.
Sorry for the back and forth, let me know if you rather prefer to move on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, it looks like simba
+ wide
has some issues with no_std
👀 https://github.com/Ogeon/palette/actions/runs/14018720715/job/39248266654?pr=431
…std compatibility
Implements the angle/bool_mask/num traits for simba's
Wide*
wrapper types behind asimba
feature flag.Note: to activate the implementation, both the
wide
and thesimba
feature must be enabled.Closed Issues
Wide*
types of thesimba
crate #430, by implementing palette's "primitive" traits for simba's wide wrappers