Add compound assignment and ThreeWayCompare operations with tests#11
Conversation
There was a problem hiding this comment.
Pull request overview
Adds support for three-way comparison (<=>) and compound assignment operators (+=, -=, *=, /=) for primitive<>, integrating them into the existing operations/dispatcher pipeline and extending the basic operations test suite.
Changes:
- Introduce
ThreeWayCompareoperation, runtime invoker support, andoperator<=>returningstd::expected<Ordering, error_kind>. - Add compound assignment operator overloads wired through a shared
apply_assignhelper. - Add unit tests covering ordering results for integers/floats and compound assignment success/failure for division by zero.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
tests/basic/test_operations.cpp |
Adds tests for <=> (strong/partial ordering + bool error) and compound assignment operator behavior. |
src/operations/operators.cppm |
Implements three_way_compare, operator<=>, and compound assignment operators via apply_assign. |
src/operations/invoker.cppm |
Adds runtime implementation for ThreeWayCompare and binds it for checked/unchecked/saturating value policies. |
src/operations/impl.cppm |
Introduces the ThreeWayCompare operation tag and its operation traits. |
| EXPECT_EQ(result.error(), policy::error::kind::divide_by_zero); | ||
| EXPECT_EQ(value.load(), 100); | ||
| } | ||
|
|
There was a problem hiding this comment.
Current compound-assignment tests only exercise same-type operands (default policy::type::strict). Since operator+=/-=/... are templated for arbitrary primitive_instance pairs and policy::type::compatible/transparent can negotiate a wider common_rep, add tests that cover mixed-type compound assignment and the expected behavior when the computed result is not representable in the LHS type (e.g. error + LHS unchanged).
| TEST(OperationsTest, | |
| CompoundAssignmentSupportsMixedTypesWithCompatibleTypePolicy) { | |
| using namespace mcpplibs::primitives::operators; | |
| using lhs_t = primitive<short, policy::value::checked, | |
| policy::error::expected, policy::type::compatible>; | |
| using rhs_t = primitive<int, policy::value::checked, | |
| policy::error::expected, policy::type::compatible>; | |
| auto value = lhs_t{10}; | |
| auto add_result = (value += rhs_t{32}); | |
| ASSERT_TRUE(add_result.has_value()); | |
| EXPECT_EQ(value.load(), 42); | |
| EXPECT_EQ(add_result->value(), 42); | |
| } | |
| TEST(OperationsTest, | |
| CompoundAssignmentKeepsLhsOnMixedTypeOverflowWithCompatibleTypePolicy) { | |
| using namespace mcpplibs::primitives::operators; | |
| using lhs_t = | |
| primitive<std::int8_t, policy::value::checked, policy::error::expected, | |
| policy::type::compatible>; | |
| using rhs_t = primitive<int, policy::value::checked, | |
| policy::error::expected, policy::type::compatible>; | |
| auto value = lhs_t{static_cast<std::int8_t>(120)}; | |
| auto add_result = (value += rhs_t{120}); | |
| ASSERT_FALSE(add_result.has_value()); | |
| EXPECT_EQ(add_result.error(), policy::error::kind::overflow); | |
| EXPECT_EQ(value.load(), static_cast<std::int8_t>(120)); | |
| } |
|
|
||
| auto const assigned_rep = static_cast<lhs_rep>(out->load()); | ||
| lhs.store(underlying::traits<lhs_value_type>::from_rep(assigned_rep)); | ||
| return out; |
There was a problem hiding this comment.
apply_assign converts the computed common_rep result to lhs_rep via static_cast and then stores it into lhs. If the negotiated common_rep is wider than lhs_rep (e.g. policy::type::compatible with int8_t += int), this can silently narrow/wrap (or become implementation-defined for signed) and bypass the library’s checked-value overflow/underflow guarantees. Consider either constraining apply_assign to cases where common_rep == lhs_rep, or validating that the result is representable in lhs_value_type (and returning an overflow/underflow error) before mutating lhs.
| auto out = apply<OpTag, Lhs, Rhs, ErrorPayload>(lhs, rhs); | ||
| if (!out.has_value()) { | ||
| return std::unexpected(out.error()); | ||
| } | ||
|
|
||
| auto const assigned_rep = static_cast<lhs_rep>(out->load()); | ||
| lhs.store(underlying::traits<lhs_value_type>::from_rep(assigned_rep)); | ||
| return out; | ||
| } |
There was a problem hiding this comment.
apply_assign returns out (a primitive<common_rep>) even though it stores a potentially converted/narrowed value into lhs. When common_rep differs from lhs’s representation, the returned value may not match what was actually stored in lhs, which is surprising for compound assignment. Consider returning a result that reflects the post-assignment lhs value/type, or ensuring the stored value is identical to out (by disallowing/narrowing-checking mismatched reps).
No description provided.