From 875f143d7466453483a904824d0cd6cfe5cad0c2 Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Sat, 25 Aug 2018 13:31:16 +1200 Subject: [PATCH] fix parameter ordering inconsistency that could cause switching the parameters to give different results --- source/detail/numeric_utils.hpp | 13 +++++++----- tests/detail/numeric_util_test_suite.cpp | 27 ++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 5 deletions(-) diff --git a/source/detail/numeric_utils.hpp b/source/detail/numeric_utils.hpp index 30f49f89..25d0dc67 100644 --- a/source/detail/numeric_utils.hpp +++ b/source/detail/numeric_utils.hpp @@ -88,18 +88,19 @@ bool float_equals(const LNumber &lhs, const RNumber &rhs, int epsilon_scale = 20) // scale the "fuzzy" equality. Higher value gives a more tolerant comparison { + // a type that lhs and rhs can agree on + using common_t = typename std::common_type::type; + // asserts for sane usage static_assert(std::is_floating_point::value || std::is_floating_point::value, "Using this function with two integers is just wasting time. Use =="); - static_assert(std::is_floating_point::value, - "Cannot extract epsilon from a number that isn't a floating point type"); + static_assert(std::numeric_limits::epsilon() < common_t{1}, + "epsilon >= 1.0 will cause all comparisons to return true"); // NANs always compare false with themselves if (std::isnan(lhs) || std::isnan(rhs)) { return false; } - // a type that lhs and rhs can agree on - using common_t = typename std::common_type::type; // epsilon type defaults to float because even if both args are a higher precision type // either or both could have been promoted by prior operations // if a higher precision is required, the template type can be changed @@ -108,7 +109,9 @@ float_equals(const LNumber &lhs, const RNumber &rhs, // epsilon for numeric_limits is valid when abs(x) <1.0, scaling only needs to be upwards // in particular, this prevents a lhs of 0 from requiring an exact comparison // additionally, a scale factor is applied. - common_t scaled_fuzz = epsilon_scale * epsilon * max(xlnt::detail::abs(lhs), common_t{1}); + common_t scaled_fuzz = epsilon_scale * epsilon * max(max(xlnt::detail::abs(lhs), + xlnt::detail::abs(rhs)), // |max| of parameters. + common_t{1}); // clamp return ((lhs + scaled_fuzz) >= rhs) && ((rhs + scaled_fuzz) >= lhs); } diff --git a/tests/detail/numeric_util_test_suite.cpp b/tests/detail/numeric_util_test_suite.cpp index a6bf810a..e79d507e 100644 --- a/tests/detail/numeric_util_test_suite.cpp +++ b/tests/detail/numeric_util_test_suite.cpp @@ -31,6 +31,7 @@ public: { register_test(test_float_equals_zero); register_test(test_float_equals_large); + register_test(test_float_equals_fairness); register_test(test_min); register_test(test_max); register_test(test_abs); @@ -121,6 +122,32 @@ public: xlnt_assert(!xlnt::detail::float_equals(nan, 1000.f)); } + void test_float_equals_fairness() + { + // tests for parameter ordering dependency + // (lhs ~= rhs) == (rhs ~= lhs) + const double test_val = 1.0; + const double test_diff_pass = 1.192092e-07; // should all pass with this + const double test_diff = 1.192093e-07; // difference enough to provide different results if the comparison is not "fair" + const double test_diff_fails = 1.192094e-07; // should all fail with this + + // test_diff_pass + xlnt_assert(xlnt::detail::float_equals((test_val + test_diff_pass), test_val, 1)); + xlnt_assert(xlnt::detail::float_equals(test_val, (test_val + test_diff_pass), 1)); + xlnt_assert(xlnt::detail::float_equals(-(test_val + test_diff_pass), -test_val, 1)); + xlnt_assert(xlnt::detail::float_equals(-test_val, -(test_val + test_diff_pass), 1)); + // test_diff + xlnt_assert(xlnt::detail::float_equals((test_val + test_diff), test_val, 1)); + xlnt_assert(xlnt::detail::float_equals(test_val, (test_val + test_diff), 1)); + xlnt_assert(xlnt::detail::float_equals(-(test_val + test_diff), -test_val, 1)); + xlnt_assert(xlnt::detail::float_equals(-test_val, -(test_val + test_diff), 1)); + // test_diff_fails + xlnt_assert(!xlnt::detail::float_equals((test_val + test_diff_fails), test_val, 1)); + xlnt_assert(!xlnt::detail::float_equals(test_val, (test_val + test_diff_fails), 1)); + xlnt_assert(!xlnt::detail::float_equals(-(test_val + test_diff_fails), -test_val, 1)); + xlnt_assert(!xlnt::detail::float_equals(-test_val, -(test_val + test_diff_fails), 1)); + } + void test_min() { // simple