search

linebender / kurbo

A Rust library for manipulating curves
Apache License 2.0
716 stars 69 forks source link

Make the FloatFuncs trait a sealed public trait, add sin/sinf. #332

Closed ratmice closed 8 months ago

ratmice commented 8 months ago

This trait, with the addition of sin/sinf is needed to build peniko with the --no-default-features --features libm combination (added in the pr below).

The approach taken here is that rather than duplicating this common module in peniko, to make it pub, and add the sin/sinf. for peniko. This trait is only conditionally defined so use of it in peniko would look something like:

+#[cfg(not(feature = "std"))]
+#[allow(unused_imports)]
+use kurbo::common::FloatFuncs as _

This patch is in regards to fixing the testsuite failures for default features added in https://github.com/linebender/peniko/pull/23

ratmice commented 8 months ago

One thing this patch doesn't try to fix is cargo test --no-default-features --features libm I manually checked that the respective build target works, don't know see that this combination is actually being tested by ci though.

It is perhaps worth adding that. Edit: Nevermind I see now it is being checked on one of the macos runs

richard-uk1 commented 8 months ago

Hey @ratmice sorry if this is a silly question, but why do we need to seal the trait?

ratmice commented 8 months ago

I just did it in the spirit of it being the more conservative change, i.e. it is easy to unseal it later, but sealing it would be a breaking change.

So honestly no good reason.

richard-uk1 commented 8 months ago

I just did it in the spirit of it being the more conservative change, i.e. it is easy to unseal it later, but sealing it would be a breaking change.

@ratmice Would you mind putting a comment on the sealing trait along these lines? Then future me knows we can remove the sealing if we need to :). Other than that LGTM.

ratmice commented 8 months ago

If it's alright i'll squash/force push this all down to one commit with the initial commit message.

DJMcNab commented 8 months ago

That would be alright, although that will also happen as part of the merge process

ratmice commented 8 months ago

I'm happy with it as is, and ran the peniko patch on top of this revision through it's CI.

I don't believe anything in a peniko review would require changes here, but feel like I should wait for at least a cursory a review there until landing this here.