From cfa9a710de356cce0909a72ea489255ead6b4303 Mon Sep 17 00:00:00 2001 From: Alex Wang Date: Sun, 26 Mar 2023 16:28:58 -0400 Subject: [PATCH] Fix units::modf() for scaled quantities with a frac part units::modf() appears to add a spurious scaling factor to the fractional result it returns. For example, this test code: percent<> pval{202.5}; double pmodfr1; decltype(pval) pmodfr2; EXPECT_EQ(std::modf(pval.to(), &pmodfr1), units::modf(pval, &pmodfr2)); EXPECT_EQ(pmodfr1, pmodfr2); Fails with this message: units/unitTests/main.cpp:4649: Failure Expected equality of these values: std::modf(pval.to(), &pmodfr1) Which is: 0.025 units::modf(pval, &pmodfr2) Which is: 0.00025 [ FAILED ] UnitMath.modf (0 ms) Not 100% confident I understand the specifics of the C++ rules here, but I think this line from units::modf() is the culprit: dimensionlessUnit fracpart = std::modf(x.template to(), &intp); I think this uses uses copy-initialization to initialize fracpart by using the implicit converting constructor for linear dimensionless units to create a dimensionlessUnit from the std::modf() result, then using that to direct-initialize fracpart. The problem is that because this uses the implicit converting constructor for linear dimensionless units, the scaling done to produce a value to pass to std::modf is not "undone" when reconstituting fracpart. Instead, the std::modf() result is taken as-is to produce a dimensionlessUnit value, effectively resulting in the scaling factor applying twice by the time the units::modf() result is turned back into a double. For example, passing percent<>{202.5} to units::modf() results in 2.025 being passed to std::modf(), which returns 0.025. This is passed to the converting constructor, resulting in percent<>{0.025} (!), which is then used to direct-initialize fracpart, which is then returned. This is then turned into a double in EXPECT_EQ(), applying the percent<> scaling factor once again to get a final value of 0.00025. This can be fixed by first making a dimensionless<> from the std::modf() result, which results in the unit converting constructor being called, which ensures the scaling factor is reapplied when initializing fracpart. intpart is not affected by this issue because operator=() more or less already does this by making a dimensionless<> from its argument before changing *this. --- include/units/core.h | 2 +- unitTests/main.cpp | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/include/units/core.h b/include/units/core.h index c1cdaed0..bd119469 100644 --- a/include/units/core.h +++ b/include/units/core.h @@ -3654,7 +3654,7 @@ namespace units dimensionless modf(const dimensionlessUnit x, dimensionlessUnit* intpart) noexcept { typename dimensionlessUnit::underlying_type intp; - dimensionlessUnit fracpart = std::modf(x.template to(), &intp); + dimensionlessUnit fracpart = dimensionless<>{std::modf(x.template to(), &intp)}; *intpart = intp; return fracpart; } diff --git a/unitTests/main.cpp b/unitTests/main.cpp index 8e516ecd..373d2c0c 100644 --- a/unitTests/main.cpp +++ b/unitTests/main.cpp @@ -4642,6 +4642,12 @@ TEST_F(UnitMath, modf) double umodfr1; decltype(uval) umodfr2; EXPECT_EQ(std::modf(uval.to(), &umodfr1), units::modf(uval, &umodfr2)); + EXPECT_EQ(umodfr1, umodfr2); + percent<> pval{202.5}; + double pmodfr1; + decltype(pval) pmodfr2; + EXPECT_EQ(std::modf(pval.to(), &pmodfr1), units::modf(pval, &pmodfr2)); + EXPECT_EQ(pmodfr1, pmodfr2); } TEST_F(UnitMath, exp2)