Skip to content

Commit

Permalink
Fix units::modf() for scaled quantities with a frac part
Browse files Browse the repository at this point in the history
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<double>(), &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<double>(), &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<decltype(intp)>(), &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.
  • Loading branch information
Alex Wang committed Mar 26, 2023
1 parent 70d5e3f commit a3b6c26
Show file tree
Hide file tree
Showing 2 changed files with 7 additions and 1 deletion.
2 changes: 1 addition & 1 deletion include/units/core.h
Original file line number Diff line number Diff line change
Expand Up @@ -3654,7 +3654,7 @@ namespace units
dimensionless<typename dimensionlessUnit::underlying_type> modf(const dimensionlessUnit x, dimensionlessUnit* intpart) noexcept
{
typename dimensionlessUnit::underlying_type intp;
dimensionlessUnit fracpart = std::modf(x.template to<decltype(intp)>(), &intp);
dimensionlessUnit fracpart = dimensionless<>{std::modf(x.template to<decltype(intp)>(), &intp)};
*intpart = intp;
return fracpart;
}
Expand Down
6 changes: 6 additions & 0 deletions unitTests/main.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4642,6 +4642,12 @@ TEST_F(UnitMath, modf)
double umodfr1;
decltype(uval) umodfr2;
EXPECT_EQ(std::modf(uval.to<double>(), &umodfr1), units::modf(uval, &umodfr2));
EXPECT_EQ(umodfr1, umodfr2);
percent<> pval{202.5};
double pmodfr1;
decltype(pval) pmodfr2;
EXPECT_EQ(std::modf(pval.to<double>(), &pmodfr1), units::modf(pval, &pmodfr2));
EXPECT_EQ(pmodfr1, pmodfr2);
}

TEST_F(UnitMath, exp2)
Expand Down

0 comments on commit a3b6c26

Please sign in to comment.