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 zero constants/methods for each quantity #26

Closed
iliekturtles opened this issue Apr 16, 2017 · 9 comments · Fixed by #250
Closed

Add zero constants/methods for each quantity #26

iliekturtles opened this issue Apr 16, 2017 · 9 comments · Fixed by #250

Comments

@iliekturtles
Copy link
Owner

Add constant? Add const fn? impl Zero?

radix added a commit to radix/uom that referenced this issue Dec 23, 2017
radix added a commit to radix/uom that referenced this issue Dec 23, 2017
iliekturtles pushed a commit that referenced this issue Dec 23, 2017
iliekturtles added a commit that referenced this issue Dec 26, 2017
@iliekturtles
Copy link
Owner Author

iliekturtles commented Jan 14, 2019

impl Zero done in #41. Adding a zero constant or const fn is more complicated because trait functions cannot be const. Need some way to write const fn zero<V>() -> V so that const fn zero() -> Quantity<..., V> can be written.

@iliekturtles iliekturtles added this to the v1.0.0 milestone Jun 29, 2019
@iliekturtles iliekturtles removed this from the v1.0.0 milestone Jul 8, 2019
@svartalf
Copy link
Contributor

svartalf commented Sep 2, 2019

Since the uom minimal Rust supported version will be bumped to 1.31 (at least it seems so based on the dev-macros branch), it would be nice to make the {Quantity}::new method const too, it would allow to define typed constants, ex.

const UNIX_EPOCH_DELTA: Time = Time::new::<time::second>(11644473600.0);

EDIT: I'm starting to think that this was a wrong issue to add this comment :) Do you want me to create a new issue?

@iliekturtles
Copy link
Owner Author

#165 added!

It's not pretty, but you can define constants right now as follows:

const UNIX_EPOCH_DELTA: Time = Time { dimension: PhantomData, units: PhantomData, value: 11644473600.0, };

@Michael-F-Bryan
Copy link

Michael-F-Bryan commented Apr 15, 2020

What about adding something like Quantity::new_in_base_unit()?

impl<D, U> Quantity<D, U> {
    pub const fn new_in_base_units(value: U) -> Self {
        dimension: PhantomData, 
        units: PhantomData, 
        value,
    }
}

That way we'd be able to write const ZERO_VELOCITY: Velocity = Velocity::new_in_base_units(0.0).

It's not ideal, but at least it gives people a way to create Quantity constants and is better than people directly assigning the dimension and units fields. Then when const fn in traits becomes stable, the Quantity::new() constructor can do the generic conversion and defer to Quantity::new_in_base_units().

@iliekturtles
Copy link
Owner Author

That's an excellent idea. new_in_base_unit or new_with_base_unit?

If someone wants to submit a PR add the new method just below fn new. I'll make some time in the next few days to add the method otherwise.

uom/src/quantity.rs

Lines 292 to 307 in 409252e

/// Create a new quantity from the given value and measurement unit.
///
/// ## Generic Parameters
/// * `N`: Unit.
#[inline(always)]
pub fn new<N>(v: V) -> Self
where
N: Unit + $crate::Conversion<V, T = V::T>,
{
$quantity {
dimension: $crate::lib::marker::PhantomData,
units: $crate::lib::marker::PhantomData,
value: super::to_base::<Dimension, U, V, N>(&v),
}
}

@Michael-F-Bryan
Copy link

I don't think it's possible with the way Quantity is currently defined.

   ::: src/system.rs:243:9
    |
243 | /         pub struct Quantity<D, U, V>
244 | |         where
245 | |             D: Dimension + ?Sized,
246 | |             U: Units<V> + ?Sized,
...   |
256 | |             pub value: V,
257 | |         }
    | |_________- required by `si::Quantity`

Placing $quantity::new_in_base_units() below $quantity::new() (in the same impl block) hits "error[E0723]: trait bounds other than Sized on const fn parameters are unstable". That impl block requires U: Units<V> and V: Num + Conversion<V>.

Because the struct definition contains a where clause, any function creating a Quantity also needs to have the same where clause. Putting $quantity::new_in_base_units() in its own impl block gives me 312 instances of the following:

error[E0277]: the trait bound `U: si::Units<V>` is not satisfied
   --> src/quantity.rs:290:56
    |
290 |               pub const fn new_in_base_unit(value: V) -> Self {
    |                                                          ^^^^
    |
   ::: src/si/volume_rate.rs:3:1
    |
3   | / quantity! {
4   | |     /// Volume rate (base unit cubic meter per second, m³ · s⁻¹).
5   | |     quantity: VolumeRate; "volume rate";
6   | |     /// Dimension of volume rate, L³T⁻¹ (base unit cubic meter per second, m³ · s⁻¹).
...   |
149 | |     }
150 | | }
    | |_- in this macro invocation
    |
   ::: src/system.rs:243:9
    |
243 | /         pub struct Quantity<D, U, V>
244 | |         where
245 | |             D: Dimension + ?Sized,
246 | |             U: Units<V> + ?Sized,
...   |
256 | |             pub value: V,
257 | |         }
    | |_________- required by `si::Quantity`
    |
    = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)
help: consider restricting type parameter `U`
    |
287 |         impl<U: si::Units<V>, V> $quantity<U, V> {
    |               ^^^^^^^^^^^^^^

I know the Rust API guidelines recommend not putting where clauses on a generic struct's declaration and only placing it on impl blocks, but to change that now would be a breaking change and a making a new major release for a single const fn constructor is probably not worth it.

@iliekturtles
Copy link
Owner Author

So my few days apparently turned into 20 days. I haven't forgotten about this and it's still on my to-do list. I just haven't made time to get to it yet.

@ryanpeach
Copy link

Whats the status of this issue?

@iliekturtles
Copy link
Owner Author

Still starred in my inbox waiting for me to get to it unfortunately. I'm trying to get #229 merged. That in turn is blocked while I'm trying to update dependencies which uncovered some floating point precision issues.

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 a pull request may close this issue.

4 participants