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

Remove c'tor from underlying types #434

Closed
BenFrantzDale opened this issue Feb 1, 2023 · 4 comments
Closed

Remove c'tor from underlying types #434

BenFrantzDale opened this issue Feb 1, 2023 · 4 comments

Comments

@BenFrantzDale
Copy link

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
Copy link
Owner

mpusz commented Feb 2, 2023

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
Copy link
Collaborator

chiphogg commented Feb 3, 2023

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
Copy link
Author

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
Copy link
Collaborator

chiphogg commented Feb 3, 2023

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. 🙂

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

3 participants