Skip to content

Commit

Permalink
fix: compound assignment operations on quantities now behave the same…
Browse files Browse the repository at this point in the history 
… as on the underying representation types

Resolves #137
  • Loading branch information
@mpusz
mpusz committed Oct 29, 2024
1 parent 797ae2e commit 7445585
Show file tree
Hide file tree
Showing 3 changed files with 50 additions and 23 deletions.
40 changes: 25 additions & 15 deletions src/core/include/mp-units/framework/quantity.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -377,40 +377,50 @@ class quantity {
}

// compound assignment operators
template<typename FwdQ, std::derived_from<quantity> Q = std::remove_cvref_t<FwdQ>>
requires requires(rep a, rep b) {
template<typename FwdQ, auto R2, typename Rep2, std::derived_from<quantity> Q = std::remove_cvref_t<FwdQ>>
requires detail::QuantityConvertibleTo<quantity<R2, rep>, quantity> && requires(rep a, Rep2 b) {
{ a += b } -> std::same_as<rep&>;
}
friend constexpr decltype(auto) operator+=(FwdQ&& lhs, const quantity& rhs)
friend constexpr decltype(auto) operator+=(FwdQ&& lhs, const quantity<R2, Rep2>& rhs)
{
lhs.numerical_value_is_an_implementation_detail_ += rhs.numerical_value_is_an_implementation_detail_;
if constexpr (equivalent(unit, get_unit(R2)))
lhs.numerical_value_is_an_implementation_detail_ += rhs.numerical_value_is_an_implementation_detail_;
else
lhs.numerical_value_is_an_implementation_detail_ += rhs.in(lhs.unit).numerical_value_is_an_implementation_detail_;
return std::forward<FwdQ>(lhs);
}

template<typename FwdQ, std::derived_from<quantity> Q = std::remove_cvref_t<FwdQ>>
requires requires(rep a, rep b) {
template<typename FwdQ, auto R2, typename Rep2, std::derived_from<quantity> Q = std::remove_cvref_t<FwdQ>>
requires detail::QuantityConvertibleTo<quantity<R2, rep>, quantity> && requires(rep a, Rep2 b) {
{ a -= b } -> std::same_as<rep&>;
}
friend constexpr decltype(auto) operator-=(FwdQ&& lhs, const quantity& rhs)
friend constexpr decltype(auto) operator-=(FwdQ&& lhs, const quantity<R2, Rep2>& rhs)
{
lhs.numerical_value_is_an_implementation_detail_ -= rhs.numerical_value_is_an_implementation_detail_;
if constexpr (equivalent(unit, get_unit(R2)))
lhs.numerical_value_is_an_implementation_detail_ -= rhs.numerical_value_is_an_implementation_detail_;
else
lhs.numerical_value_is_an_implementation_detail_ -= rhs.in(lhs.unit).numerical_value_is_an_implementation_detail_;
return std::forward<FwdQ>(lhs);
}

template<typename FwdQ, std::derived_from<quantity> Q = std::remove_cvref_t<FwdQ>>
requires(!treat_as_floating_point<rep>) && requires(rep a, rep b) {
{ a %= b } -> std::same_as<rep&>;
}
friend constexpr decltype(auto) operator%=(FwdQ&& lhs, const quantity& rhs)
template<typename FwdQ, auto R2, typename Rep2, std::derived_from<quantity> Q = std::remove_cvref_t<FwdQ>>
requires detail::QuantityConvertibleTo<quantity<R2, rep>, quantity> && (!treat_as_floating_point<rep>) &&
requires(rep a, Rep2 b) {
{ a %= b } -> std::same_as<rep&>;
}
friend constexpr decltype(auto) operator%=(FwdQ&& lhs, const quantity<R2, Rep2>& rhs)

{
MP_UNITS_EXPECTS_DEBUG(rhs != zero());
lhs.numerical_value_is_an_implementation_detail_ %= rhs.numerical_value_is_an_implementation_detail_;
if constexpr (equivalent(unit, get_unit(R2)))
lhs.numerical_value_is_an_implementation_detail_ %= rhs.numerical_value_is_an_implementation_detail_;
else
lhs.numerical_value_is_an_implementation_detail_ %= rhs.in(lhs.unit).numerical_value_is_an_implementation_detail_;
return std::forward<FwdQ>(lhs);
}

template<typename FwdQ, typename Value, std::derived_from<quantity> Q = std::remove_cvref_t<FwdQ>>
requires(!Quantity<Value>) && requires(rep a, const Value b) {
requires(!Quantity<Value>) && requires(rep a, Value b) {
{ a *= b } -> std::same_as<rep&>;
}
friend constexpr decltype(auto) operator*=(FwdQ&& lhs, const Value& v)
Expand Down
4 changes: 2 additions & 2 deletions test/static/chrono_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -137,8 +137,8 @@ static_assert(std::chrono::nanoseconds(quantity{1 * ns}) == 1ns);
static_assert(std::chrono::nanoseconds(quantity{1 * s}) == 1s);

// operators
static_assert((1 * s += 1s) == 2 * s);
static_assert((2 * s -= 1s) == 1 * s);
static_assert((1 * s += quantity{1s}) == 2 * s);
static_assert((2 * s -= quantity{1s}) == 1 * s);
static_assert(quantity{1s} + 1 * s == 2 * s);
static_assert(quantity{1s} + 1 * min == 61 * s);
static_assert(1 * s + quantity{1s} == 2 * s);
Expand Down
29 changes: 23 additions & 6 deletions test/static/quantity_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -456,7 +456,7 @@ static_assert((1 * m *= 2 * one).numerical_value_in(m) == 2);
static_assert((2 * m /= 2 * one).numerical_value_in(m) == 1);
static_assert((7 * m %= 2 * m).numerical_value_in(m) == 1);

// different types
// different representation types
static_assert((2.5 * m += 3 * m).numerical_value_in(m) == 5.5);
static_assert((123 * m += 1 * km).numerical_value_in(m) == 1123);
static_assert((5.5 * m -= 3 * m).numerical_value_in(m) == 2.5);
Expand All @@ -467,12 +467,26 @@ static_assert((2.5 * m *= 3 * one).numerical_value_in(m) == 7.5);
static_assert((7.5 * m /= 3 * one).numerical_value_in(m) == 2.5);
static_assert((3500 * m %= 1 * km).numerical_value_in(m) == 500);

// convertible quantity types
static_assert((isq::length(1 * m) += isq::height(1 * m)).numerical_value_in(m) == 2);
static_assert((isq::length(2 * m) -= isq::height(1 * m)).numerical_value_in(m) == 1);
static_assert((isq::length(7 * m) %= isq::height(2 * m)).numerical_value_in(m) == 1);

// different representation types with truncation
// clang-format off
static_assert((3 * m += 2.5 * m).numerical_value_in(m) == []{ auto v = 3; v += 2.5; return v; }());
static_assert((3 * m -= 1.5 * m).numerical_value_in(m) == []{ auto v = 3; v -= 1.5; return v; }());
static_assert((2 * m *= 2.5).numerical_value_in(m) == []{ auto v = 2; v *= 2.5; return v; }());
static_assert((10 * m /= 2.5).numerical_value_in(m) == []{ auto v = 10; v /= 2.5; return v; }());
static_assert((2 * m *= 2.5 * one).numerical_value_in(m) == []{ auto v = 2; v *= 2.5; return v; }());
static_assert((10 * m /= 2.5 * one).numerical_value_in(m) == []{ auto v = 10; v /= 2.5; return v; }());
// clang-format on

// static_assert((std::uint8_t{255} * m %= 256 * m).numerical_value_in(m) == [] {
// std::uint8_t ui(255);
// return ui %= 256;
// }()); // UB
// TODO: Fix
static_assert((std::uint8_t{255}* m %= 257 * m).numerical_value_in(m) != [] {
static_assert((std::uint8_t{255}* m %= 257 * m).numerical_value_in(m) == [] {
std::uint8_t ui(255);
return ui %= 257;
}());
Expand All @@ -490,15 +504,18 @@ static_assert((22 * m /= 3.33 * one).numerical_value_in(m) == 6);
template<template<auto, typename> typename Q>
concept invalid_compound_assignments = requires() {
// truncating not allowed
requires !requires(Q<isq::length[m], int> l) { l += 2.5 * m; };
requires !requires(Q<isq::length[m], int> l) { l -= 2.5 * m; };
requires !requires(Q<isq::length[km], int> l) { l += 2 * isq::length[m]; };
requires !requires(Q<isq::length[km], int> l) { l -= 2 * isq::length[m]; };
requires !requires(Q<isq::length[km], int> l) { l %= 2 * isq::length[m]; };
requires !requires(Q<isq::length[km], int> l) { l %= 2 * percent; };
requires !requires(Q<isq::length[km], int> l) { l %= 2. * percent; };

// TODO: accept non-truncating argument
// compound assignment with a non-convertible quantity not allowed
requires !requires(Q<isq::height[m], int> l) { l += 2 * isq::length[m]; };
requires !requires(Q<isq::height[m], int> l) { l -= 2 * isq::length[m]; };
requires !requires(Q<isq::height[m], int> l) { l %= 2 * isq::length[m]; };

// dimensionless quantities with a unit different than `one`
requires !requires(Q<isq::length[km], int> l) { l *= 1 * (km / m); };
requires !requires(Q<isq::length[km], int> l) { l /= 1 * (km / m); };
requires !requires(Q<isq::length[km], int> l) { l %= 1 * (km / m); };
Expand Down

2 comments on commit 7445585

@JohelEGP
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is allowing truncation with += and -= consistent with where we don't allow it?
It's OK to reject those for consistency.
The problem before was that truncation happened in the caller,
which gave a different result than the operation on the representation types.
Assuming common types,
a @ b should either work and have the same numerical value as a.numerical_value @ b.numerical_value,
or it shouldn't work.

@mpusz
Copy link
Owner Author

@mpusz mpusz commented on 7445585 Oct 30, 2024

Choose a reason for hiding this comment

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

Ahh, you are probably right. I changed all of the rules for compound assignment operators to prevent narrowing.

Please sign in to comment.