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

Option<Float> Niche optimization #483

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

novacrazy
Copy link
Contributor

@novacrazy novacrazy commented Mar 24, 2024

Implements #283 as both a raw Option<f32> -> ArchivedOptionF32 low-level wrapper and as a with::Niche/ArchiveWith implementation using noisy_float::NoisyFloat for safety.

noisy_float is an optional dependency, and a simple FloatChecker trait has been included with option_float for non-noisy_float usage, but this is automatically implemented for all noisy_float::FloatChecker types if enabled.

Some of the generated docs are bit off with the lowercase type names, so I may fix those later. Fixed.

Future work may be to include all F: Float types instead of only f32 and f64, but that would require work with rend or others to implement Float or whatnot.

Starting this as a draft so I can review it again in the morning and fix anything I missed.

Closes #283

@djkoloski
Copy link
Collaborator

Hmm, I see the problem here; we don't really have a niche-able float type to bridge from. I really like your approach of adding a checker type parameter that decides whether a value is valid or not, I think that we might even want to generalize it. I could see something like this:

union NichedOption<T, D> {
    some: T,
    none: (),
}

trait Decider<T> {
    /// # Safety
    ///
    /// `ptr` must be non-null, properly-aligned, and be valid for reads. `ptr` does not have to point to a valid `T`.
    unsafe fn is_some(ptr: *const T) -> bool;
}

Which should allow us to generalize the Niche wrapper:

struct Niche<D>(PhantomData<D>);

struct Zero;

impl Decider<NonZeroU32> for Zero { .. }

struct NaN;

impl Decider<f32> for NaN { .. }

// etc.

It would be really nice to unify all of this niching code. What do you think? I understand if you'd rather not pivot like this, since it's a pretty significant change in scope.

@djkoloski djkoloski force-pushed the main branch 3 times, most recently from aa99556 to 39c03e6 Compare May 20, 2024 03:44
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.

Add a non-NaN/non-Inf niched float wrapper
2 participants