From a223c08a39ff583a87898c926c9a6d712f8b5f99 Mon Sep 17 00:00:00 2001
From: Mateusz Pusz <mateusz.pusz@gmail.com>
Date: Thu, 28 Nov 2024 23:02:11 +0100
Subject: [PATCH] refactor: multiplication and division by scalars was a bad
 idea for `Complex` and `Vector`

---
 .../framework/representation_concepts.h       | 82 +++++++++----------
 test/static/concepts_test.cpp                 | 10 +++
 2 files changed, 50 insertions(+), 42 deletions(-)

diff --git a/src/core/include/mp-units/framework/representation_concepts.h b/src/core/include/mp-units/framework/representation_concepts.h
index 414c08752..384479a72 100644
--- a/src/core/include/mp-units/framework/representation_concepts.h
+++ b/src/core/include/mp-units/framework/representation_concepts.h
@@ -168,22 +168,18 @@ constexpr bool disable_complex = false;
 namespace detail {
 
 template<typename T>
-concept Complex =
-  (!disable_complex<T>) && WeaklyRegular<T> && Scalar<value_type_t<T>> &&
-  std::constructible_from<T, value_type_t<T>, value_type_t<T>> && requires(T a, T b, value_type_t<T> s) {
-    // complex operations
-    { -a } -> std::common_with<T>;
-    { a + b } -> std::common_with<T>;
-    { a - b } -> std::common_with<T>;
-    { a* b } -> std::common_with<T>;
-    { a / b } -> std::common_with<T>;
-    { a* s } -> std::common_with<T>;
-    { s* a } -> std::common_with<T>;
-    { a / s } -> std::common_with<T>;
-    ::mp_units::real(a);
-    ::mp_units::imag(a);
-    ::mp_units::modulus(a);
-  };
+concept Complex = (!disable_complex<T>) && WeaklyRegular<T> && requires(T a, T b) {
+  // complex operations
+  { -a } -> std::common_with<T>;
+  { a + b } -> std::common_with<T>;
+  { a - b } -> std::common_with<T>;
+  { a* b } -> std::common_with<T>;
+  { a / b } -> std::common_with<T>;
+  ::mp_units::real(a);
+  ::mp_units::imag(a);
+  ::mp_units::modulus(a);
+  requires std::constructible_from<T, decltype(::mp_units::real(a)), decltype(::mp_units::imag(a))>;
+};
 
 namespace magnitude_impl {
 
@@ -233,23 +229,19 @@ constexpr bool disable_vector = false;
 namespace detail {
 
 template<typename T>
-concept Vector =
-  (!disable_vector<T>) && WeaklyRegular<T> && Scalar<value_type_t<T>> && requires(T a, T b, value_type_t<T> s) {
-    // vector operations
-    { -a } -> std::common_with<T>;
-    { a + b } -> std::common_with<T>;
-    { a - b } -> std::common_with<T>;
-    { a* s } -> std::common_with<T>;
-    { s* a } -> std::common_with<T>;
-    { a / s } -> std::common_with<T>;
-    ::mp_units::magnitude(a);
-    // TODO should we check for the below as well (e.g., when `size() > 1` or `2`)
-    // { zero_vector<T>() } -> Vector;
-    // { unit_vector(a) } -> Vector;
-    // { scalar_product(a, b) } -> Scalar;
-    // { vector_product(a, b) } -> Vector;
-    // { tensor_product(a, b) } -> Tensor2;
-  };
+concept Vector = (!disable_vector<T>) && WeaklyRegular<T> && requires(T a, T b) {
+  // vector operations
+  { -a } -> std::common_with<T>;
+  { a + b } -> std::common_with<T>;
+  { a - b } -> std::common_with<T>;
+  ::mp_units::magnitude(a);
+  // TODO should we also check for the below (e.g., when `size() > 1` or `2`)
+  // { zero_vector<T>() } -> Vector;
+  // { unit_vector(a) } -> Vector;
+  // { scalar_product(a, b) } -> Scalar;
+  // { vector_product(a, b) } -> Vector;
+  // { tensor_product(a, b) } -> Tensor2;
+};
 
 // TODO provide when some actual operations will be required
 // template<typename T>
@@ -266,27 +258,33 @@ constexpr bool is_quantity = false;
 template<typename T>
 using scaling_factor_type_t = conditional<treat_as_floating_point<T>, long double, std::intmax_t>;
 
+// TODO how can we use `(!Quantity<T>)` below?
 template<typename T>
-concept ScalarRepresentation = (!is_quantity<T>) && Scalar<T> && requires(T a, T b, scaling_factor_type_t<T> f) {
+concept ScalarRepresentation = (!is_quantity<T>) && Scalar<T> && requires(T v, scaling_factor_type_t<T> f) {
   // scaling
-  { a* f } -> std::common_with<T>;
-  { f* a } -> std::common_with<T>;
-  { a / f } -> std::common_with<T>;
+  { v* f } -> std::common_with<T>;
+  { f* v } -> std::common_with<T>;
+  { v / f } -> std::common_with<T>;
 };
 
 template<typename T>
-concept ComplexRepresentation = (!is_quantity<T>) && Complex<T> && requires(T a, T b, scaling_factor_type_t<T> f) {
+concept ComplexRepresentation = (!is_quantity<T>) && Complex<T> && requires(T v, scaling_factor_type_t<T> f) {
   // scaling
   // TODO The below conversion to `T` is an exception compared to other representation types
   // `std::complex<T>` * `U` do not work, but `std::complex<T>` is convertible from `U`
   // Maybe expose this as a customization point?
-  { a* T(f) } -> std::common_with<T>;
-  { T(f) * a } -> std::common_with<T>;
-  { a / T(f) } -> std::common_with<T>;
+  { v* T(f) } -> std::common_with<T>;
+  { T(f) * v } -> std::common_with<T>;
+  { v / T(f) } -> std::common_with<T>;
 };
 
 template<typename T>
-concept VectorRepresentation = (!is_quantity<T>) && Vector<T>;
+concept VectorRepresentation = (!is_quantity<T>) && Vector<T> && requires(T v, scaling_factor_type_t<T> f) {
+  // scaling
+  { v* f } -> std::common_with<T>;
+  { f* v } -> std::common_with<T>;
+  { v / f } -> std::common_with<T>;
+};
 
 // template<typename T>
 // concept TensorRepresentation = (!is_quantity<T>) && Tensor<T>;
diff --git a/test/static/concepts_test.cpp b/test/static/concepts_test.cpp
index d0d4552b0..2ee71fbd6 100644
--- a/test/static/concepts_test.cpp
+++ b/test/static/concepts_test.cpp
@@ -434,4 +434,14 @@ static_assert(!QuantityPointLike<quantity<isq::time[si::second]>>);
 static_assert(!QuantityPointLike<quantity_point<si::metre, my_origin>>);
 static_assert(!QuantityPointLike<int>);
 
+// Quantity Characters
+
+static_assert(detail::Scalar<quantity<one>>);
+static_assert(detail::Scalar<quantity<one, int>>);
+static_assert(!detail::Scalar<quantity_point<one>>);
+static_assert(!detail::Scalar<quantity_point<si::metre>>);
+// TODO it would be make the below work
+static_assert(!detail::Scalar<quantity<si::metre>>);
+static_assert(!detail::Scalar<quantity<isq::speed[si::metre / si::second], int>>);
+
 }  // namespace