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

Add const getter for Copy types #126

Open
helgoboss opened this issue Feb 20, 2024 · 4 comments
Open

Add const getter for Copy types #126

helgoboss opened this issue Feb 20, 2024 · 4 comments

Comments

@helgoboss
Copy link

I want to propose optionally adding a const getter method to newtypes that wrap a Copy value.

Motivation

Given the following newtype:

/// Represents a tempo measured in beats per minute.
#[nutype(
    new_unchecked,
    validate(finite, greater_or_equal = 1.0, less_or_equal = 960.0),
    derive(
        Copy,
        Clone,
        Eq,
        PartialEq,
        Ord,
        PartialOrd,
        Debug,
        Default,
        Display,
        FromStr,
        Into,
        TryFrom
    ),
    default = 1.0
)]
pub struct Bpm(f64);

impl Bpm {
    /// The minimum possible value.
    pub const MIN: Self = unsafe { Self::new_unchecked(1.0) };

    /// The maximum possible value.
    pub const MAX: Self = unsafe { Self::new_unchecked(960.0) };
}

(Side note: Being able to call Self::new_unchecked from a const context needs this)

Now I would like to do things like this:

const BPM_SPAN: f64 = Bpm::MAX.into_inner() - Bpm::MIN.into_inner();

Above code is currently not possible because into_inner() is not const:

            pub fn into_inner(self) -> #inner_type {
                self.0
            }

Thoughts

Just making it const in general unfortunately doesn't work. Because it takes self by value (which is the correct choice for a method of this name). And that fails to compile if the wrapped type is heap-allocated ("the destructor for this type cannot be evaluated in constant functions").

I see 2 possibilities.

a) Making into_inner const, but only for Copy types

            pub const fn into_inner(self) -> #inner_type {
                self.0
            }

b) Introducing a separate const get method, but only for Copy types

            pub const fn get(self) -> #inner_type {
                self.0
            }

Personally, I prefer b. There are quite many examples in the standard library where a wrapper exposes its wrapped Copy value in a get method, so it appears to be a good practice. But ultimately I don't mind.

Do you think it's somehow possible to do a or b? Just for Copy types? And if yes, would you consider it? I would be happy to write a PR.

@helgoboss
Copy link
Author

Addition: I have the feeling that it would be hard doing something special just for Copy types. If this is too hard or even impossible, adding the const getter just to integers and floating point wrappers would be the alternative - that would cover most of the use cases I guess.

@helgoboss
Copy link
Author

Here's a prototypical implementation that adds pub const fn get(self) -> #inner_type if the newtype itself derives Copy. What do you think about this approach? I could turn it into a proper PR including tests if you are fine with it.

helgoboss@e186e09

@greyblake
Copy link
Owner

@helgoboss Hi, thanks for your report!

I think it's partially overlaps with #35

The problem with adding new method .get() is that it typically returns a reference, not a value itself. I don't also want to have some extra methods for not real reason. If you need a reference, you could use AsRef trait, but year it won't work with const I guess.

So I'd prefer rather option A, meaning implementing const fn into_inner() for integer and float types.
If you want you can go ahead and open a PR for that. Just please make sure to add proper tests.

@helgoboss
Copy link
Author

Ok, I understand. I hope to open a PR soon.

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

No branches or pull requests

2 participants