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

Support integral underlying types #126

Merged
merged 25 commits into from
May 25, 2018

Conversation

JohelEGP
Copy link
Contributor

@JohelEGP JohelEGP commented May 12, 2018

Resolves #125.

  • Prevent lossy initialization of units from arithmetic types.
  • Prevent lossy conversions and truncation of units in its converting constructor.
  • Provide an alternative to convert that carries intermediate computations in the widest representation and has a type safe interface.
  • Specialize std::common_type for units of the same dimension. The common type should be the least precise unit that both units can be exactly converted to.
  • Implement the modulus operators with the new framework.
  • Use the new framework in the equality operators.
  • Use the new framework in the relational operators.
  • Use the new framework in the arithmetic linear operators.
  • Use the new framework in the arithmetic decibel operators.
  • Use the new framework in the cmath functions.

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

@nholthaus
Copy link
Owner

looking awesome so far.

@JohelEGP
Copy link
Contributor Author

JohelEGP commented May 16, 2018

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 JohelEGP force-pushed the duration_like_unit branch 4 times, most recently from dd0ea2e to e490fc9 Compare May 17, 2018 00:53
@JohelEGP
Copy link
Contributor Author

JohelEGP commented May 17, 2018

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.

@JohelEGP JohelEGP force-pushed the duration_like_unit branch 4 times, most recently from 35a7c01 to 4320117 Compare May 17, 2018 19:40
@GElliott
Copy link

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

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

@JohelEGP
Copy link
Contributor Author

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

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 JohelEGP force-pushed the duration_like_unit branch 2 times, most recently from 9d858c0 to 5a5ef15 Compare May 18, 2018 01:19
@JohelEGP
Copy link
Contributor Author

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.

@JohelEGP JohelEGP force-pushed the duration_like_unit branch 2 times, most recently from 1f1f324 to 23685b9 Compare May 19, 2018 21:33
include/units.h Outdated
units::unit<
units::unit_conversion<
units::detail::ratio_gcd<typename UnitConversionLhs::conversion_ratio, typename UnitConversionRhs::conversion_ratio>,
typename UnitConversionLhs::dimension_type,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if using dimension_of_t would be most correct here.

@JohelEGP JohelEGP force-pushed the duration_like_unit branch 4 times, most recently from ba3aa75 to c826471 Compare May 20, 2018 01:41
@JohelEGP
Copy link
Contributor Author

JohelEGP commented May 24, 2018

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:

  • When the macros for defining units are reworked (Show off the power of the new variadic dimensions system #124), they should be underlying type aware. The operator<< and to_string functions only work on UNIT_LIB_DEFAULT_TYPE units, potentially performing lossy conversions. There's also the operator<< for dB units.
  • Consider making integer unit literals return their units with an integer underlying type.
  • Investigate having constants of different underlying types. These might be useful for users who can only use float, for example. There have been proposals for standarization regarding that.
  • Consider deprecating UNIT_LIB_DEFAULT_TYPE and substituting its uses with double.

@JohelEGP
Copy link
Contributor Author

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.

@JohelEGP JohelEGP force-pushed the duration_like_unit branch 2 times, most recently from aa7d66d to 47f42e4 Compare May 24, 2018 23:40
@nholthaus nholthaus merged commit 428c54b into nholthaus:v3.x May 25, 2018
@JohelEGP JohelEGP deleted the duration_like_unit branch May 25, 2018 22:01
JohelEGP added a commit to JohelEGP/units that referenced this pull request Jul 8, 2018
This macro is part of the public API.
As such, the definitions are in terms
of the strong unit alias.
If an object of it degenerates into its base,
something else might be printed.
In practice, this might not be a problem.
If the unit is a program-defined unit,
the base might print the right thing.
If it is a strong unit alias
over a library-defined unit,
something different, but correct nonetheless,
might be printed. See
nholthaus#126 (comment).
JohelEGP added a commit to JohelEGP/units that referenced this pull request Jul 10, 2018
This macro is part of the public API.
As such, the definitions are in terms
of the strong unit alias.
If an object of it degenerates into its base,
something else might be printed.
In practice, this might not be a problem.
If the unit is a program-defined unit,
the base might print the right thing.
If it is a strong unit alias
over a library-defined unit,
something different, but correct nonetheless,
might be printed. See
nholthaus#126 (comment).
JohelEGP added a commit to JohelEGP/units that referenced this pull request Jul 10, 2018
This macro is part of the public API.
As such, the definitions are in terms
of the strong unit alias.
If an object of it degenerates into its base,
something else might be printed.
In practice, this might not be a problem.
If the unit is a program-defined unit,
the base might print the right thing.
If it is a strong unit alias
over a library-defined unit,
something different, but correct nonetheless,
might be printed. See
nholthaus#126 (comment).
JohelEGP added a commit to JohelEGP/units that referenced this pull request Jul 14, 2018
This macro is part of the public API.
As such, the definitions are in terms
of the strong unit alias.
If an object of it degenerates into its base,
something else might be printed.
In practice, this might not be a problem.
If the unit is a program-defined unit,
the base might print the right thing.
If it is a strong unit alias
over a library-defined unit,
something different, but correct nonetheless,
might be printed. See
nholthaus#126 (comment).
@JohelEGP JohelEGP mentioned this pull request Jul 18, 2018
JohelEGP added a commit to JohelEGP/units that referenced this pull request Jul 21, 2018
This macro is part of the public API.
As such, the definitions are in terms
of the strong unit alias.
If an object of it degenerates into its base,
something else might be printed.
In practice, this might not be a problem.
If the unit is a program-defined unit,
the base might print the right thing.
If it is a strong unit alias
over a library-defined unit,
something different, but correct nonetheless,
might be printed. See
nholthaus#126 (comment).
@coveralls
Copy link

Coverage Status

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

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 this pull request may close these issues.

4 participants