nholthaus / units

a compile-time, header-only, dimensional analysis and unit conversion library built on c++14 with no dependencies.
http://nholthaus.github.io/units/
MIT License
955 stars 135 forks source link

Fix units::modf() for scaled quantities with a frac part #312

Open ts826848 opened 1 year ago

ts826848 commented 1 year ago

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.

ts826848 commented 1 year ago

I suppose I have a related question - the unit constructor for built-in types and operator=() for built-in types behave differently, since the former uses the provided value as-is while the latter first converts it to a dimensionless<> before changing *this. For example, this test code:

percent<> p1{5.0};
percent<> p2;
p2 = 5.0;
EXPECT_EQ(p1, p2);

Fails with this message:

Expected equality of these values:
  p1
    Which is: 5 pct
  p2
    Which is: 500 pct

Is this difference intended?