nholthaus / units

a compile-time, header-only, dimensional analysis and unit conversion library built on c++14 with no dependencies.
http://nholthaus.github.io/units/
MIT License
952 stars 135 forks source link

Support integral underlying types #126

Closed JohelEGP closed 6 years ago

JohelEGP commented 6 years ago

Resolves #125.

Along the way, some of the supporting traits and other utilities might need to be changed to support mixed underlying types.

nholthaus commented 6 years ago

looking awesome so far.

JohelEGP commented 6 years ago

75eb9ef introduces unit_conversion_t as an alias for an unit_conversion whose dimension is always a dimension_t. This is done to prevent anomalies like unit_conversion<std::ratio<1>, unit_conversion<std::ratio<1>, dimension::length>>. This addition flattens the permitted unit_conversions. This allows implementing a specialization of std::common_type that Just Works. It also reduces the length of the error messages thanks to the flattening. And maybe other things thanks to the increased consistency.

You'll see the addition of a few macros. The flattening allowed me to discover a few units that before were equivalent, e.g. meter_t and unit<unit_conversion<std::ratio<1>, length::meter>, double>, but were different types simply because their UnitType parameter were different types. The fact that they're now the same type means that the definition of units introduce redefinitions, so the new macros avoid those.

I'll add the documentation later.

JohelEGP commented 6 years ago

The unit_conversion_t approach (c449ade ) didn't quite work. The library defines units like metric_ton, which is an alias for unit_conversion<std::ratio<1000>, kilograms>. That's equivalent to megagrams, but not the same. This allows the library to have a strong type for megagrams, which this change would break. Some things I noticed is that unit_conversion<std::ratio<1>, metric_ton> counts as an ephemeral unit, and falls back to streaming this unit in terms of kg instead of the t abbreviation. And that ephemeral units don't have name or abbreviation.

I couldn't come up with a change that provides the benefits of both. And it wasn't even necessary to do so. I thought it'd be nice to use std::is_same in the tests for std::common_type, but I'll just use the traits of the library instead.

GElliott commented 6 years ago

I've been hacking up changes to v2.3 to make it easier to mix units with underlying types (my use-case is outlined at the end of this issue), and I see some overlap with this work. For example, this would let you add a float-based meter type to a double-based meter type. Should we coordinate?

JohelEGP commented 6 years ago

Sure! We could do it here or in my fork.

JohelEGP commented 6 years ago

I have reread your message (I had done so before, back when I was contemplating someday using this library). The changes I'm going to do will indeed allow you to do 1 and 2, but 3 is not part of this effort. What does the library do that impedes its units being standard layout?

Regarding your remark about duration_cast, in your listed example you can indeed do the assignment from and to float and double durations (and units, with the second commit of this PR). However, I agree that std::chrono might have gone overboard. We have the convert to do these explicit casts, but we could also have explicit constructors for such lossy conversions.

JohelEGP commented 6 years ago

I had been struggling with formulating a specialization of std::common_type that worked for units with pi exponent and translation, and only just realized that conceptually their greatest common divisor does the job, just like the regular conversion ratio. I really didn't want to continue working on the other tasks before getting this done, otherwise I feared I'd have to limit the effort to the units without those ratios. Now all that's left is including the non-linear scale in the definition.

JohelEGP commented 6 years ago

I think that does it. Now I can continue with the tasks. Any bug with the specialization of std::common_type should come up during the work on the arithmetic operators.

nholthaus commented 6 years ago

still looking beautiful. I almost cried (with joy) when I saw that you refactored the unit tests away from the VS2013 compliant mess they were before, that was not something I was looking forward to.

JohelEGP commented 6 years ago

That was essential to prove that the type safe convert works correctly.

nholthaus commented 6 years ago

They were happy tears :) This pull is going to be the most important thing that's happened to units in years.

JohelEGP commented 6 years ago

I think it's done. If anything, I should add some tests for the last five items in the lists ("Use the new framework in"), which only used the existing tests, which don't account for mixed-underlying types units.

Now unused:

Further work:

JohelEGP commented 6 years ago

still looking beautiful. I almost cried (with joy) when I saw that you refactored the unit tests away from the VS2013 compliant mess they were before, that was not something I was looking forward to.

It can be refactored further. From convert<millimeter_t>(meter_t(0.001)) to millimeter_t(meter_t(0.001)). The later is implemented in terms of convert and reflects the guideline of preferring implicit conversions.

coveralls commented 4 years ago

Coverage Status

Coverage decreased (-6.7%) to 93.04% when pulling cb58be583eee95a2448ee7fafdbc3b5471193831 on johelegp:duration_like_unit into 57324b8d5d5590943a27e5da546f964401754f9a on nholthaus:v3.x.