Skip to content

Commit

Permalink
Add MSVC coverage for remaining suspicious asserts (#365)
Browse files Browse the repository at this point in the history
I went through every `static_assert` in our C++ files, and added MSVC
test coverage for any that looked like they might be vulnerable to the
same error as in #363.

I added these test cases one at a time, so I could tell which ones
failed.

- The `is_known_to_be_less_than_one` "bug" never actually failed,
  whether I called it directly or indirectly.  I'm leaving the tests in
  place, and I "fixed" it anyway because it made the function body more
  stylistically consistent.

- The `inverse_as`/`inverse_in` bug was indeed another instance of this
  problem.  Now we have coverage and a fix.

Fixes #364.
  • Loading branch information
chiphogg authored Dec 18, 2024
1 parent 3074486 commit 8165cb6
Show file tree
Hide file tree
Showing 3 changed files with 9 additions and 3 deletions.
4 changes: 2 additions & 2 deletions au/code/au/apply_rational_magnitude_to_integral.hh
Original file line number Diff line number Diff line change
Expand Up @@ -75,9 +75,9 @@ constexpr T clamp_to_range_of(U x) {
//

template <typename... BPs>
constexpr bool is_known_to_be_less_than_one(Magnitude<BPs...> m) {
constexpr bool is_known_to_be_less_than_one(Magnitude<BPs...>) {
using MagT = Magnitude<BPs...>;
static_assert(is_rational(m), "Magnitude must be rational");
static_assert(is_rational(MagT{}), "Magnitude must be rational");

constexpr auto num_result = get_value_result<std::uintmax_t>(numerator(MagT{}));
static_assert(num_result.outcome == MagRepresentationOutcome::OK,
Expand Down
2 changes: 1 addition & 1 deletion au/code/au/math.hh
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,7 @@ constexpr auto inverse_in(TargetUnits target_units, Quantity<U, R> q) {
constexpr auto UNITY = make_constant(UnitProductT<>{});

static_assert(
UNITY.in<R>(associated_unit(target_units) * U{}) >= threshold ||
UNITY.in<R>(associated_unit(TargetUnits{}) * U{}) >= threshold ||
std::is_floating_point<R>::value,
"Dangerous inversion risking truncation to 0; must supply explicit Rep if truly desired");

Expand Down
6 changes: 6 additions & 0 deletions single-file-test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ using namespace au;
using ::au::symbols::m;
using ::au::symbols::s;

constexpr auto ns = ::au::nano(s);

// This ad hoc utility is a stand-in for GTEST, which we can't use here.
template <typename ExpectedT, typename ActualT>
bool expect_equal(ExpectedT expected, ActualT actual) {
Expand All @@ -45,6 +47,10 @@ int main(int argc, char **argv) {
{
expect_equal((meters / second)(5) * seconds(6), meters(30)),
expect_equal(SPEED_OF_LIGHT.as<int>(m / s), 299'792'458 * m / s),
expect_equal(detail::is_known_to_be_less_than_one(mag<5>() / mag<7>()), true),
expect_equal(detail::is_known_to_be_less_than_one(mag<7>() / mag<5>()), false),
expect_equal((10 * m).coerce_in(m * mag<5>() / mag<7>()), 14),
expect_equal(inverse_as(ns, 333 / s), 3'003'003 * ns),
},
};
return std::all_of(std::begin(results), std::end(results), [](auto x) { return x; }) ? 0 : 1;
Expand Down

0 comments on commit 8165cb6

Please sign in to comment.