From 00d182e133ea9e47939907a5381e76cb7ca09c69 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Johel=20Ernesto=20Guerrero=20Pe=C3=B1a?=
 <johelegp@gmail.com>
Date: Sat, 6 Feb 2021 02:20:00 -0400
Subject: [PATCH] fix: operators to behave like the underlying type's

---
 src/include/units/quantity.h                 | 50 ++++++++++++++------
 src/include/units/quantity_kind.h            | 23 ++-------
 test/unit_test/static/quantity_kind_test.cpp | 23 ++++++++-
 test/unit_test/static/quantity_test.cpp      | 37 ++++++++++++++-
 test/unit_test/static/unit_constants.cpp     | 25 ++++++----
 5 files changed, 111 insertions(+), 47 deletions(-)

diff --git a/src/include/units/quantity.h b/src/include/units/quantity.h
index 574318a7..d4f60ea3 100644
--- a/src/include/units/quantity.h
+++ b/src/include/units/quantity.h
@@ -150,10 +150,11 @@ public:
   [[nodiscard]] constexpr rep count() const noexcept { return value_; }
 
   // member unary operators
-  [[nodiscard]] constexpr quantity operator+() const
-    requires requires(rep v) { { +v } -> std::same_as<rep>; }
+  [[nodiscard]] constexpr Quantity auto operator+() const
+    requires requires(rep v) { { +v } -> std::common_with<rep>; }
   {
-    return *this;
+    using ret = quantity<D, U, decltype(+count())>;
+    return ret(+count());
   }
 
   [[nodiscard]] constexpr Quantity auto operator-() const
@@ -233,9 +234,10 @@ public:
     return *this;
   }
 
-  constexpr quantity& operator%=(const rep& rhs)
-    requires (!floating_point_<rep>) &&
-             requires(rep a, const rep b) { { a %= b } -> std::same_as<rep&>; }
+  template<typename Rep2>
+  constexpr quantity& operator%=(const Rep2& rhs)
+    requires (!floating_point_<rep>) && (!floating_point_<Rep2>) &&
+             requires(rep a, const Rep2 b) { { a %= b } -> std::same_as<rep&>; }
   {
     value_ %= rhs;
     return *this;
@@ -251,16 +253,34 @@ public:
 
   // Hidden Friends
   // Below friend functions are to be found via argument-dependent lookup only
-  [[nodiscard]] friend constexpr quantity operator+(const quantity& lhs, const quantity& rhs)
-    requires invoke_result_convertible_to_<rep, std::plus<>, rep, rep>
+  template<typename Value>
+  [[nodiscard]] friend constexpr Quantity auto operator+(const quantity& lhs, const Value& rhs)
+    requires (!Quantity<Value>) && is_same_v<unit, units::one> &&
+          invoke_result_convertible_to_<rep, std::plus<>, rep, Value>
   {
-    return quantity(lhs.count() + rhs.count());
+    return units::quantity(lhs.count() + rhs);
+  }
+  template<typename Value>
+  [[nodiscard]] friend constexpr Quantity auto operator+(const Value& lhs, const quantity& rhs)
+    requires (!Quantity<Value>) && is_same_v<unit, units::one> &&
+          invoke_result_convertible_to_<rep, std::plus<>, Value, rep>
+  {
+    return units::quantity(lhs + rhs.count());
   }
 
-  [[nodiscard]] friend constexpr quantity operator-(const quantity& lhs, const quantity& rhs)
-    requires invoke_result_convertible_to_<rep, std::minus<>, rep, rep>
+  template<typename Value>
+  [[nodiscard]] friend constexpr Quantity auto operator-(const quantity& lhs, const Value& rhs)
+    requires (!Quantity<Value>) && is_same_v<unit, units::one> &&
+          invoke_result_convertible_to_<rep, std::minus<>, rep, Value>
   {
-    return quantity(lhs.count() - rhs.count());
+    return units::quantity(lhs.count() - rhs);
+  }
+  template<typename Value>
+  [[nodiscard]] friend constexpr Quantity auto operator-(const Value& lhs, const quantity& rhs)
+    requires (!Quantity<Value>) && is_same_v<unit, units::one> &&
+          invoke_result_convertible_to_<rep, std::minus<>, Value, rep>
+  {
+    return units::quantity(lhs - rhs.count());
   }
 
   template<typename Value>
@@ -312,11 +332,11 @@ public:
     return ret(q.count() % v);
   }
 
-  [[nodiscard]] friend constexpr quantity operator%(const quantity& lhs, const quantity& rhs)
-    requires (!floating_point_<rep>) &&
+  [[nodiscard]] friend constexpr Quantity auto operator%(const quantity& lhs, const quantity& rhs)
+    requires (!floating_point_<rep>) && is_same_v<unit, units::one> &&
             invoke_result_convertible_to_<rep, std::modulus<>, rep, rep>
   {
-    return quantity(lhs.count() % rhs.count());
+    return units::quantity(lhs.count() % rhs.count());
   }
 
   [[nodiscard]] friend constexpr auto operator<=>(const quantity& lhs, const quantity& rhs)
diff --git a/src/include/units/quantity_kind.h b/src/include/units/quantity_kind.h
index 6dc5ed66..71b968b1 100644
--- a/src/include/units/quantity_kind.h
+++ b/src/include/units/quantity_kind.h
@@ -189,8 +189,9 @@ public:
     return *this;
   }
 
-  constexpr quantity_kind& operator%=(const rep& rhs)
-    requires requires(quantity_type q) { q %= rhs; }
+  template<typename Rep2>
+  constexpr quantity_kind& operator%=(const Rep2& rhs)
+    requires (!Quantity<Rep2>) && requires(quantity_type q, const Rep2 r) { q %= r; }
   {
     q_ %= rhs;
     return *this;
@@ -205,18 +206,6 @@ public:
 
   // Hidden Friends
   // Below friend functions are to be found via argument-dependent lookup only
-  [[nodiscard]] friend constexpr quantity_kind operator+(const quantity_kind& lhs, const quantity_kind& rhs)
-    requires requires { lhs.common() + rhs.common(); }
-  {
-    return quantity_kind(lhs.common() + rhs.common());
-  }
-
-  [[nodiscard]] friend constexpr quantity_kind operator-(const quantity_kind& lhs, const quantity_kind& rhs)
-    requires requires { lhs.common() - rhs.common(); }
-  {
-    return quantity_kind(lhs.common() - rhs.common());
-  }
-
   template<QuantityValue Value>
   [[nodiscard]] friend constexpr QuantityKind auto operator*(const quantity_kind& qk, const Value& v)
     requires requires { { qk.common() * v } -> Quantity; }
@@ -252,12 +241,6 @@ public:
     return detail::make_quantity_kind<quantity_kind>(qk.common() % v);
   }
 
-  [[nodiscard]] friend constexpr quantity_kind operator%(const quantity_kind& lhs, const quantity_kind& rhs)
-    requires requires { lhs.common() % rhs.common(); }
-  {
-    return quantity_kind(lhs.common() % rhs.common());
-  }
-
   [[nodiscard]] friend constexpr auto operator<=>(const quantity_kind& lhs, const quantity_kind& rhs)
     requires std::three_way_comparable<quantity_type>
   {
diff --git a/test/unit_test/static/quantity_kind_test.cpp b/test/unit_test/static/quantity_kind_test.cpp
index cb10afd5..301e687d 100644
--- a/test/unit_test/static/quantity_kind_test.cpp
+++ b/test/unit_test/static/quantity_kind_test.cpp
@@ -386,11 +386,18 @@ static_assert([]() {
   w = width<metre, int>(3 * m);
   assert(&(w *= 3.9) == &w && w.common() == 11 * m);
   assert(&(w /= 3.9) == &w && w.common() == 2 * m);
-  assert(&(w %= 3.9) == &w && w.common() == 2 * m);
   return true;
 }());
 #endif
 
+static_assert((std::uint8_t(255) * m %= 256) == (width<metre, std::uint8_t>(255 * m) %= 256).common());
+// static_assert((std::uint8_t(255) * m %= 256 * m) !=
+//   (width<metre, std::uint8_t>(255 * m) %= width<metre, std::uint8_t>(256 * m)).common());  // UB
+static_assert((std::uint8_t(255) * m %= 257) == (width<metre, std::uint8_t>(255 * m) %= 257).common());
+// TODO: Fix
+static_assert((std::uint8_t(255) * m %= 257 * m) ==
+  (width<metre, std::uint8_t>(255 * m) %= width<metre, std::uint8_t>(257 * m)).common());
+
 static_assert(same((-width<metre, short>(short{1} * m)).common(), int{-1} * m));
 
 template<typename K, typename U, typename Qx>
@@ -405,6 +412,7 @@ template<typename Width>
 concept invalid_compound_assignments = requires(quantity_kind<Width, metre, int> w) {
   requires !requires { w += 1; };
   requires !requires { w -= 1; };
+  requires !requires { w %= 1.0; };
   requires !requires { w %= w * 1.0; };
   requires invalid_compound_assignments_<Width, metre, length<metre, int>>;
   requires invalid_compound_assignments_<Width, metre, height<metre, int>>;
@@ -429,6 +437,15 @@ static_assert(same(width<metre, int>(2 * m) - width<metre, double>(3. * m), widt
 static_assert(same(width<metre, double>(2. * m) - width<metre, int>(3 * m), width<metre, double>(-1. * m)));
 static_assert(same(width<metre, double>(2e3 * m) - width<kilometre, int>(3 * km), width<metre, double>(-1e3 * m)));
 
+static_assert(is_same_v<
+  decltype((width<metre, std::uint8_t>(0 * m) + width<metre, std::uint8_t>(0 * m)).common().count()), int>);
+static_assert(is_same_v<
+  decltype((width<metre, std::uint8_t>(0 * m) - width<metre, std::uint8_t>(0 * m)).common().count()), int>);
+static_assert((width<metre, std::uint8_t>(128 * m) + width<metre, std::uint8_t>(128 * m)).common().count() ==
+  std::uint8_t(128) + std::uint8_t(128));
+static_assert((width<metre, std::uint8_t>(0 * m) - width<metre, std::uint8_t>(1 * m)).common().count() ==
+  std::uint8_t(0) - std::uint8_t(1));
+
 static_assert(!std::is_invocable_v<std::plus<>, width<metre>, double>);
 static_assert(!std::is_invocable_v<std::plus<>, width<metre>, length<metre>>);
 static_assert(!std::is_invocable_v<std::plus<>, width<metre>, quantity_point<dim_length, metre>>);
@@ -493,6 +510,10 @@ static_assert(same(((2 * m) / height<metre, int>(3 * m) * (0 * m)), height<metre
 static_assert(same(width<metre, int>(2 * m) % 3, width<metre, int>(2 * m)));
 static_assert(same(width<metre, int>(3 * m) % width<metre, int>(2 * m), width<metre, int>(1 * m)));
 
+static_assert(is_same_v<
+  decltype((width<metre, std::uint8_t>(0 * m) % width<metre, std::uint8_t>(0 * m)).common().count()),
+  decltype(std::uint8_t(0) % std::uint8_t(0))>);
+
 static_assert(!std::is_invocable_v<std::multiplies<>, width<metre>, width<metre>>);
 static_assert(!std::is_invocable_v<std::multiplies<>, width<metre>, height<metre>>);
 static_assert(!std::is_invocable_v<std::multiplies<>, height<metre>, quantity_point<dim_length, metre>>);
diff --git a/test/unit_test/static/quantity_test.cpp b/test/unit_test/static/quantity_test.cpp
index 050187cd..9952dfa5 100644
--- a/test/unit_test/static/quantity_test.cpp
+++ b/test/unit_test/static/quantity_test.cpp
@@ -30,6 +30,7 @@
 #include "units/physical/si/fps/derived/speed.h"
 #include <chrono>
 #include <complex>
+#include <cstdint>
 #include <mutex>
 #include <string>
 #include <utility>
@@ -265,6 +266,8 @@ static_assert([](auto v) { auto vv = ++v; return std::pair(v, vv); }(123_q_m) ==
 static_assert([](auto v) { auto vv = v--; return std::pair(v, vv); }(123_q_m) == std::pair(122_q_m, 123_q_m));
 static_assert([](auto v) { auto vv = --v; return std::pair(v, vv); }(123_q_m) == std::pair(122_q_m, 122_q_m));
 
+static_assert(is_same_v<decltype((+(short{0} * m)).count()), int>);
+
 
 ////////////////////////
 // compound assignment
@@ -291,6 +294,12 @@ static_assert((2.5_q_m *= quantity(3)).count() == 7.5);
 static_assert((7.5_q_m /= quantity(3)).count() == 2.5);
 static_assert((3500_q_m %= 1_q_km).count() == 500);
 
+static_assert((std::uint8_t(255) * m %= 256).count() == [] { std::uint8_t ui(255); return ui %= 256; }());
+// static_assert((std::uint8_t(255) * m %= 256 * m).count() != [] { std::uint8_t ui(255); return ui %= 256; }());  // UB
+static_assert((std::uint8_t(255) * m %= 257).count() == [] { std::uint8_t ui(255); return ui %= 257; }());
+// TODO: Fix
+static_assert((std::uint8_t(255) * m %= 257 * m).count() != [] { std::uint8_t ui(255); return ui %= 257; }());
+
 #ifndef COMP_MSVC  // TODO ICE (https://developercommunity2.visualstudio.com/t/ICE-on-a-constexpr-operator-in-mp-unit/1302907)
 // next two lines trigger conversions warnings
 // (warning disabled in CMake for this file)
@@ -369,6 +378,14 @@ static_assert(compare<decltype(1 / 1_q_s), frequency<hertz, std::int64_t>>);
 static_assert(compare<decltype(quantity{1} / 1_q_s), frequency<hertz, std::int64_t>>);
 static_assert(compare<decltype(dimensionless<percent, std::int64_t>(1) / 1_q_s), frequency<scaled_unit<ratio(1, 100), hertz>, std::int64_t>>);
 
+static_assert(is_same_v<decltype((std::uint8_t(0) * m + std::uint8_t(0) * m).count()), int>);
+static_assert(is_same_v<decltype((std::uint8_t(0) * m - std::uint8_t(0) * m).count()), int>);
+static_assert((std::uint8_t(128) * m + std::uint8_t(128) * m).count() == std::uint8_t(128) + std::uint8_t(128));
+static_assert((std::uint8_t(0) * m - std::uint8_t(1) * m).count() == std::uint8_t(0) - std::uint8_t(1));
+
+static_assert(is_same_v<decltype(((std::uint8_t(0) * m) % (std::uint8_t(0) * m)).count()),
+                        decltype(std::uint8_t(0) % std::uint8_t(0))>);
+
 // different representation types
 static_assert(is_same_v<decltype(1_q_m + 1._q_m), length<metre, long double>>);
 static_assert(is_same_v<decltype(1_q_m - 1._q_m), length<metre, long double>>);
@@ -536,6 +553,24 @@ static_assert(quantity{4} % quantity{2} == 0);
 static_assert(4 % quantity{2} == 0);
 static_assert(quantity{4} % 2 == 0);
 
+static_assert(is_same_v<decltype(quantity(0) + 0.0), decltype(quantity(0.0))>);
+static_assert(is_same_v<decltype(quantity(0) - 0.0), decltype(quantity(0.0))>);
+static_assert(is_same_v<decltype(0.0 + quantity(0)), decltype(quantity(0.0))>);
+static_assert(is_same_v<decltype(0.0 + quantity(0)), decltype(quantity(0.0))>);
+static_assert(quantity(1) + 2.3 == quantity(1 + 2.3));
+static_assert(quantity(1) - 2.3 == quantity(1 - 2.3));
+static_assert(1.2 + quantity(3) == quantity(1.2 + 3));
+static_assert(1.2 - quantity(3) == quantity(1.2 - 3));
+
+static_assert(is_same_v<decltype((quantity{std::uint8_t(0)} + quantity{std::uint8_t(0)}).count()), int>);
+static_assert(is_same_v<decltype((quantity{std::uint8_t(0)} - quantity{std::uint8_t(0)}).count()), int>);
+static_assert((quantity{std::uint8_t(128)} + quantity{std::uint8_t(128)}).count() ==
+              std::uint8_t(128) + std::uint8_t(128));
+static_assert((quantity{std::uint8_t(0)} - quantity{std::uint8_t(1)}).count() == std::uint8_t(0) - std::uint8_t(1));
+
+static_assert(is_same_v<decltype((quantity{std::uint8_t(0)} % quantity{std::uint8_t(0)}).count()),
+                        decltype(std::uint8_t(0) % std::uint8_t(0))>);
+
 
 ///////////////////////
 // equality operators
@@ -655,8 +690,6 @@ static_assert(!std::equality_comparable_with<dimensionless<one, int>, double>);
 
 template<typename Int>
 concept invalid_dimensionless_operations = requires {
-  requires !requires(dimensionless<one, Int> d) { d + 1.23; };
-  requires !requires(dimensionless<one, Int> d) { 1.23 + d; };
   requires !requires(dimensionless<percent, Int> d) { 1 + d; };
   requires !requires(dimensionless<percent, Int> d) { d + 1; };
 };
diff --git a/test/unit_test/static/unit_constants.cpp b/test/unit_test/static/unit_constants.cpp
index 35d6a17c..f16ae0c4 100644
--- a/test/unit_test/static/unit_constants.cpp
+++ b/test/unit_test/static/unit_constants.cpp
@@ -43,15 +43,22 @@ using namespace units::physical::si::unit_constants;
 
 static_assert(2 * m == 2_q_m);
 static_assert(2 * s == 2_q_s);
-#if !defined(COMP_MSVC) || defined(NDEBUG)
-static_assert([]<auto& s = ::s>() {
-  assert(!requires { s / 2; });
-  assert(!requires { s * 2; });
-  assert(!requires { s + 2; });
-  assert(!requires { s + s; });
-  return 1_q_s + s == 2_q_s;
-}());
-#endif
+template<auto& s>
+concept invalid_operations = requires {
+  requires !requires { s / 2; };
+  requires !requires { s * 2; };
+  requires !requires { s + 2; };
+  requires !requires { 2 + s; };
+  requires !requires { s + s; };
+  requires !requires { s - 2; };
+  requires !requires { 2 - s; };
+  requires !requires { s - s; };
+  requires !requires { s + 1_q_s; };
+  requires !requires { s - 1_q_s; };
+  requires !requires { 1_q_s + s; };
+  requires !requires { 1_q_s - s; };
+};
+static_assert(invalid_operations<s>);
 
 constexpr auto m_per_s = m / s;