mpusz / mp-units

The quantities and units library for C++
https://mpusz.github.io/mp-units/
MIT License
998 stars 79 forks source link

Remove c'tor from underlying types #434

Closed BenFrantzDale closed 1 year ago

BenFrantzDale commented 1 year ago

In C++, explicit isn't explicit enough. If I have

std::vector<millimeters_t> vec;
vec.emplace_back(42.0);

that's not robust against changing it to std::vector<furlongs_t> vec;; I'll wind up with 42.0 furlongs!

In my codebase I'm using a wrapper function, fromCount<Unit>(double) -> Unit. I picked "count" to match std::chrono's terminology. To be consistent with this library, I think it would be from_number<Unit>(double) -> Unit.

I'm fine with a default c'tor and with a converting c'tor (mm -> inches), but how explicit that sort of thing is is an editorial choice :-)

mpusz commented 1 year ago

I agree with that rationale. This was also already suggested by Lisa Lippincott.

Taking into account that in the V2 design, we will be able to simply create any quantity kind by multiplying a number and a unit, I think it makes a lot of sense to remove that constructor here.

As a result, a user will have to type:

std::vector<millimeters_t> vec;
vec.emplace_back(42.0 * m);
chiphogg commented 1 year ago

I can provide real-world implementation experience on this question. The pattern you're describing is what we've done with Aurora's units library from the beginning. We deleted the value constructor --- or rather, we made it private, so that only friend QuantityMaker instances can use it.

This means that if we have a QuantityD<Meters>, we can't construct it by saying QuantityD<Meters>{42.0}. We would have to say meters(42.0). meters is a QuantityMaker<Meters>, which can be called on any numeric type T, producing a Quantity<Meters, T>.

(QuantityD<Meters> is an alias for Quantity<Meters, double>.)

We have not seen major issues with this approach. The biggest issue has been that sometimes people are confused because the compiler error mentions a "private constructor". This might be improvable via an alternative mechanism of making the constructor inaccessible. FWIW, I tried deleting the constructor a while ago, and initializing the value by another approach, but IIRC the compiler errors were even worse.

The main way we handle this is by having an entry for "private constructor" in our troubleshooting guide.


tl;dr: Our implementation experience affirms that removing the constructor is a good idea.

BenFrantzDale commented 1 year ago

I can provide real-world implementation experience on this question. The pattern you're describing is what we've done with Aurora's units library from the beginning. We deleted the value constructor --- or rather, we made it private, so that only friend QuantityMaker instances can use it.

This means that if we have a QuantityD<Meters>, we can't construct it by saying QuantityD<Meters>{42.0}. We would have to say meters(42.0). meters is a QuantityMaker<Meters>, which can be called on any numeric type T, producing a Quantity<Meters, T>.

(QuantityD<Meters> is an alias for Quantity<Meters, double>.)

We have not seen major issues with this approach. The biggest issue has been that sometimes people are confused because the compiler error mentions a "private constructor". This might be improvable via an alternative mechanism of making the constructor inaccessible. FWIW, I tried deleting the constructor a while ago, and initializing the value by another approach, but IIRC the compiler errors were even worse.

The main way we handle this is by having an entry for "private constructor" in our troubleshooting guide.

tl;dr: Our implementation experience affirms that removing the constructor is a good idea.

Maybe have the private c’tor take a tag argument so the compiler doesn’t see foo{42.0} as a call to the deleted c’tor?

chiphogg commented 1 year ago

Maybe have the private c’tor take a tag argument so the compiler doesn’t see foo{42.0} as a call to the deleted c’tor?

I don't remember the precise details (I think it was over a year ago), but I do remember that I used a tag argument. Maybe I did something silly, like giving it a default value? Anyway, although my attempts to change weren't encouraging, I'm sure I didn't thoroughly explore the space of possibilities, so there may be room for improved ergonomics. :slightly_smiling_face: