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
964 stars 137 forks source link

Conversion from `std::chrono::duration<double>` to `units::seconds_t` is lossy. #291

Open BenFrantzDale opened 2 years ago

BenFrantzDale commented 2 years ago

Please include the following information in your issue:

  1. Using https://github.com/nholthaus/units/commit/ea6d126942cb3225a341568ab57ec52513977875 (Sun Oct 11 06:46:02 2020 +0800)
  2. Apple clang version 13.1.6 (clang-1316.0.21.2.5)

Related to https://github.com/nholthaus/units/issues/290

Conversion from std::chrono::duration<double> to units::seconds_t goes through std::chrono::nanoseconds, so it's inexact.

BenFrantzDale commented 2 years ago

I think changing the c'tor to this addresses the issue:

      template<class Rep, class Period, 
               class = std::enable_if_t<std::is_arithmetic<Rep>::value && traits::is_ratio<Period>::value>>
      inline constexpr unit_t(const std::chrono::duration<Rep, Period>& value) noexcept :
          nls(units::convert<unit<Period, category::time_unit>, Units>(static_cast<T>(std::chrono::duration_cast<std::chrono::duration<Rep, Period>>(value).count())))
        {
        }

although that may open a hole for egregious precision issues when using unit_t with integral value_type. Perhaps this conversion should be allowed if std::is_floating_point_v<value_type> and otherwise only if the ratio between unit_t's ratio and Period is less than std::ratio<1> (i.e., only implicit integer conversion if unit_t is finer-grained.)

But really, I just want to allow implicit conversion when the std::chrono::duration representation is exactly the same as unit_t.