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

ratmice · 2024-02-06T15:12:38Z

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
linebender/peniko#23

DJMcNab

I'm not a Kurbo maintainer, so I'm not certain that they would want this

DJMcNab · 2024-02-06T17:14:47Z

src/common.rs

@@ -5,6 +5,10 @@

 #![allow(missing_docs)]


Do we know why this is here? Does any of this new public API need docs, even just something very simple?

It looks like this is here to avoid documenting the existing pub const GAUSS_LEGENDRE_COEFFS* values.
I added basically a stub documentation explaining the trait, and pointing to the std implementation for further docs.

DJMcNab · 2024-02-06T17:19:17Z

src/common.rs

@@ -5,6 +5,10 @@

 #![allow(missing_docs)]

+mod common_sealed {


I'm not sure that common_sealed is a great name for this - I think I'd prefer something like:

mod sealed { /// Seals [super::FloatFunc] pub trait FloatFuncsSealed {} }

I don't think the Rust project documents sealed traits itself, so I think we should also probably have a link which explains it.
I don't know what link that would be, however

fixed in cb3c29d

DJMcNab · 2024-02-06T17:20:14Z

src/common.rs

@@ -14,13 +18,16 @@ macro_rules! define_float_funcs {
        => $lname:ident/$lfname:ident;
    )+) => {
        #[cfg(not(feature = "std"))]
-        pub(crate) trait FloatFuncs : Sized {
+        pub trait FloatFuncs : Sized + common_sealed::Sealed {


This should have some docs now that it's public

Should be fixed in 85ad5eb

ratmice · 2024-02-06T17:55:51Z

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

DJMcNab · 2024-02-06T18:28:22Z

src/common.rs

@@ -6,7 +6,9 @@
 #![allow(missing_docs)]

 mod sealed {
-    pub trait Sealed {}
+    /// A sealed trait which [sealed trait](https://predr.ag/blog/definitive-guide-to-sealed-traits-in-rust/) which stops this trait


I think this reads:
A sealed trait which sealed trait which..., which doesn't seem right

whoops fixed in bdfa0e7

derekdreery · 2024-02-08T16:46:37Z

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

ratmice · 2024-02-08T16:49:13Z

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.

derekdreery · 2024-02-08T16:51:18Z

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.

DJMcNab · 2024-02-08T17:02:43Z

src/common.rs

+    /// A [sealed trait](https://predr.ag/blog/definitive-guide-to-sealed-traits-in-rust/)
+    /// which stops this trait from being derived outside the library. This could be relaxed
+    /// in the future if there is are good reasons to allow outside impls.


Suggested change

/// A [sealed trait](https://predr.ag/blog/definitive-guide-to-sealed-traits-in-rust/)

/// which stops this trait from being derived outside the library. This could be relaxed

/// in the future if there is are good reasons to allow outside impls.

/// A [sealed trait](https://predr.ag/blog/definitive-guide-to-sealed-traits-in-rust/)

/// which stops [super::FloatFuncs] from being implemented outside kurbo. This could be relaxed

/// in the future if there are good reasons to allow outside impls.

I'd also probably change 'outside' to 'external' in the final line

Should really be fixed now in 4a849c3

ratmice · 2024-02-08T17:36:06Z

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

DJMcNab · 2024-02-08T18:12:02Z

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

This trait, with the addition of sin/sinf is needed to build peniko with the `--no-default-features --features libm` combination. 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.

src/common.rs

Co-authored-by: Daniel McNab <36049421+DJMcNab@users.noreply.github.com>

derekdreery

LGTM, feel free to make any changes/improvements if you'd like

ratmice · 2024-02-09T11:45:36Z

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.

ratmice mentioned this pull request Feb 6, 2024

add features for kurbo's std/libm features linebender/peniko#23

Merged

DJMcNab reviewed Feb 6, 2024

View reviewed changes

DJMcNab reviewed Feb 8, 2024

View reviewed changes

ratmice force-pushed the pub_floatfuncs branch from 6b935f2 to f1daabf Compare February 8, 2024 18:15

DJMcNab reviewed Feb 8, 2024

View reviewed changes

src/common.rs Outdated Show resolved Hide resolved

src/common.rs Outdated Show resolved Hide resolved

DJMcNab requested a review from derekdreery February 8, 2024 22:14

ratmice and others added 2 commits February 8, 2024 23:07

Update src/common.rs

2add23e

Co-authored-by: Daniel McNab <36049421+DJMcNab@users.noreply.github.com>

Update src/common.rs

2104d1e

Co-authored-by: Daniel McNab <36049421+DJMcNab@users.noreply.github.com>

derekdreery approved these changes Feb 9, 2024

View reviewed changes

ratmice merged commit 223a487 into linebender:main Feb 9, 2024
14 checks passed

ratmice deleted the pub_floatfuncs branch February 12, 2024 17:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ratmice commented Feb 6, 2024 •

edited

Loading

DJMcNab left a comment

DJMcNab Feb 6, 2024

ratmice Feb 6, 2024

DJMcNab Feb 6, 2024

ratmice Feb 6, 2024

DJMcNab Feb 6, 2024

ratmice Feb 6, 2024

ratmice commented Feb 6, 2024 •

edited

Loading

DJMcNab Feb 6, 2024

ratmice Feb 6, 2024 •

edited

Loading

derekdreery commented Feb 8, 2024

ratmice commented Feb 8, 2024

derekdreery commented Feb 8, 2024

DJMcNab Feb 8, 2024

ratmice Feb 8, 2024 •

edited

Loading

ratmice commented Feb 8, 2024

DJMcNab commented Feb 8, 2024

derekdreery left a comment •

edited

Loading

ratmice commented Feb 9, 2024

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

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

Conversation

ratmice commented Feb 6, 2024 • edited Loading

DJMcNab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ratmice commented Feb 6, 2024 • edited Loading

Choose a reason for hiding this comment

ratmice Feb 6, 2024 • edited Loading

Choose a reason for hiding this comment

derekdreery commented Feb 8, 2024

ratmice commented Feb 8, 2024

derekdreery commented Feb 8, 2024

Choose a reason for hiding this comment

ratmice Feb 8, 2024 • edited Loading

Choose a reason for hiding this comment

ratmice commented Feb 8, 2024

DJMcNab commented Feb 8, 2024

derekdreery left a comment • edited Loading

Choose a reason for hiding this comment

ratmice commented Feb 9, 2024

ratmice commented Feb 6, 2024 •

edited

Loading

ratmice commented Feb 6, 2024 •

edited

Loading

ratmice Feb 6, 2024 •

edited

Loading

ratmice Feb 8, 2024 •

edited

Loading

derekdreery left a comment •

edited

Loading