Skip to content

Commit

Permalink
Add and document explicit-rep conversion checkers (#347)
Browse files Browse the repository at this point in the history
The core enabling feature is the new "static cast checkers" library
target.  For each source and destination type, this target provides
separate checks for overflow and truncation when static casting.

We continue with our usual policy of treating floating point types as
"value preserving".  I did initially explore the possibility of treating
large integer inputs as "truncating" when converted to a floating point
type that can't represent them exactly.  However, I found this made the
library somewhat harder to reason about, for questionable benefit.
Additionally, I think it is fair to assume that users intentionally
entering the floating point domain have already accepted a kind of
"magnitude based reasoning", and trying to split hairs about preserving
exact integer values just felt too cute.

With these static cast checkers in hand, the explicit-rep runtime
conversion checkers become simple.  We check the static cast to the
common type, the unit conversion, and the final narrowing static cast to
the destination type.

To figure out how to write all these functions, I used some "fuzz-ish"
utilities, which generated random values of various integral and
floating point types, and performed various explicit-rep conversions.  I
checked that the round-trip conversion changed the value if-and-only-if
`is_conversion_lossy` was true.  After also taking intermediate sign
flips into account (to handle some signed/unsigned conversion edge
cases), I got to a point where integral-to-integral conversions always
gave the right result.  This gives me confidence in the overall
approach.  When floating point values came into the picture, I wasn't
able to design a fully satisfactory policy to avoid both false positives
and false negatives.  However, I did get to a point where the kinds of
errors I saw were ones I found acceptable, relating to "the usual
floating point error".  This was also what led me to embrace the policy
of simply treating floating point destination types as value-preserving,
consistent with the rest of the library.  This machinery is not ready
for prime time, but I posted it for posterity in the abandoned PR #346.

I also updated the documentation, including making the floating point
policy explicit.

Fixes #110.
  • Loading branch information
chiphogg authored Dec 9, 2024
1 parent c3480e8 commit 9aaeb79
Show file tree
Hide file tree
Showing 7 changed files with 581 additions and 3 deletions.
17 changes: 17 additions & 0 deletions au/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -471,6 +471,7 @@ cc_library(
":fwd",
":operators",
":rep",
":static_cast_checkers",
":unit_of_measure",
":utility",
":zero",
Expand Down Expand Up @@ -545,6 +546,22 @@ cc_test(
],
)

cc_library(
name = "static_cast_checkers",
hdrs = ["code/au/static_cast_checkers.hh"],
includes = ["code"],
)

cc_test(
name = "static_cast_checkers_test",
size = "small",
srcs = ["code/au/static_cast_checkers_test.cc"],
deps = [
":static_cast_checkers",
"@com_google_googletest//:gtest_main",
],
)

cc_library(
name = "stdx",
srcs = glob([
Expand Down
45 changes: 45 additions & 0 deletions au/code/au/quantity.hh
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
#include "au/fwd.hh"
#include "au/operators.hh"
#include "au/rep.hh"
#include "au/static_cast_checkers.hh"
#include "au/stdx/functional.hh"
#include "au/unit_of_measure.hh"
#include "au/utility/type_traits.hh"
Expand Down Expand Up @@ -646,19 +647,63 @@ constexpr bool will_conversion_overflow(Quantity<U, R> q, TargetUnitSlot target_
q.in(U{}));
}

// Check conversion for overflow (new rep).
template <typename TargetRep, typename U, typename R, typename TargetUnitSlot>
constexpr bool will_conversion_overflow(Quantity<U, R> q, TargetUnitSlot target_unit) {
// TODO(#349): Someday, we would like a more efficient implementation --- one that simply
// computes, at compile time, the smallest value that would overflow, and then compares against
// that. This version will at least let us get off the ground for now.
using Common = std::common_type_t<R, TargetRep>;
if (detail::will_static_cast_overflow<Common>(q.in(U{}))) {
return true;
}

const auto to_common = rep_cast<Common>(q);
if (will_conversion_overflow(to_common, target_unit)) {
return true;
}

const auto converted_but_not_narrowed = to_common.coerce_in(target_unit);
return detail::will_static_cast_overflow<TargetRep>(converted_but_not_narrowed);
}

// Check conversion for truncation (no change of rep).
template <typename U, typename R, typename TargetUnitSlot>
constexpr bool will_conversion_truncate(Quantity<U, R> q, TargetUnitSlot target_unit) {
return detail::ApplyMagnitudeT<R, decltype(unit_ratio(U{}, target_unit))>::would_truncate(
q.in(U{}));
}

// Check conversion for truncation (new rep).
template <typename TargetRep, typename U, typename R, typename TargetUnitSlot>
constexpr bool will_conversion_truncate(Quantity<U, R> q, TargetUnitSlot target_unit) {
using Common = std::common_type_t<R, TargetRep>;
if (detail::will_static_cast_truncate<Common>(q.in(U{}))) {
return true;
}

const auto to_common = rep_cast<Common>(q);
if (will_conversion_truncate(to_common, target_unit)) {
return true;
}

const auto converted_but_not_narrowed = to_common.coerce_in(target_unit);
return detail::will_static_cast_truncate<TargetRep>(converted_but_not_narrowed);
}

// Check for any lossiness in conversion (no change of rep).
template <typename U, typename R, typename TargetUnitSlot>
constexpr bool is_conversion_lossy(Quantity<U, R> q, TargetUnitSlot target_unit) {
return will_conversion_truncate(q, target_unit) || will_conversion_overflow(q, target_unit);
}

// Check for any lossiness in conversion (new rep).
template <typename TargetRep, typename U, typename R, typename TargetUnitSlot>
constexpr bool is_conversion_lossy(Quantity<U, R> q, TargetUnitSlot target_unit) {
return will_conversion_truncate<TargetRep>(q, target_unit) ||
will_conversion_overflow<TargetRep>(q, target_unit);
}

////////////////////////////////////////////////////////////////////////////////////////////////////
// Comparing and/or combining Quantities of different types.

Expand Down
67 changes: 67 additions & 0 deletions au/code/au/quantity_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -852,6 +852,44 @@ TEST(WillConversionOverflow, SensitiveToTypeBoundariesForPureIntegerMultiply) {
}
}

TEST(WillConversionOverflow, AlwaysFalseForQuantityEquivalentUnits) {
auto will_m_to_m_overflow = [](auto x) { return will_conversion_overflow(meters(x), meters); };

EXPECT_FALSE(will_m_to_m_overflow(2'147'483));
EXPECT_FALSE(will_m_to_m_overflow(-2'147'483));
EXPECT_FALSE(will_m_to_m_overflow(uint8_t{255}));
}

TEST(WillConversionOverflow, UnsignedToIntegralDependsOnBoundaryOfIntegral) {
EXPECT_FALSE(will_conversion_overflow<int16_t>(feet(uint16_t{65'535}), yards));

EXPECT_FALSE(will_conversion_overflow<int16_t>(feet(uint16_t{2'700}), inches));
EXPECT_TRUE(will_conversion_overflow<int16_t>(feet(uint16_t{2'800}), inches));
}

TEST(WillConversionOverflow, NegativeValuesAlwaysOverflowUnsignedDestination) {
EXPECT_TRUE(will_conversion_overflow<uint64_t>(feet(-1), inches));
EXPECT_TRUE(will_conversion_overflow<uint64_t>(feet(int8_t{-100}), yards));
}

TEST(WillConversionOverflow, SignedToUnsignedDependsOnBoundaryOfDestination) {
EXPECT_FALSE(will_conversion_overflow<uint8_t>(feet(21), inches));
EXPECT_TRUE(will_conversion_overflow<uint8_t>(feet(22), inches));
}

TEST(WillConversionOverflow, SignedToSignedHandlesNegativeAndPositiveLimits) {
EXPECT_TRUE(will_conversion_overflow<int8_t>(feet(-11), inches));
EXPECT_FALSE(will_conversion_overflow<int8_t>(feet(-10), inches));

EXPECT_FALSE(will_conversion_overflow<int8_t>(feet(10), inches));
EXPECT_TRUE(will_conversion_overflow<int8_t>(feet(11), inches));
}

TEST(WillConversionOverflow, FloatToIntHandlesLimitsOfDestType) {
EXPECT_FALSE(will_conversion_overflow<uint8_t>(feet(21.0), inches));
EXPECT_TRUE(will_conversion_overflow<uint8_t>(feet(22.0), inches));
}

TEST(WillConversionTruncate, UsesModForIntegerTypes) {
auto will_in_to_ft_truncate_i32 = [](int32_t x) {
return will_conversion_truncate(inches(x), feet);
Expand All @@ -878,6 +916,25 @@ TEST(WillConversionTruncate, UsesModForIntegerTypes) {
EXPECT_TRUE(will_in_to_ft_truncate_i32(-121));
}

TEST(WillConversionTruncate, AlwaysFalseForQuantityEquivalentUnits) {
auto will_in_to_in_truncate = [](auto x) {
return will_conversion_truncate(inches(x), inches);
};

EXPECT_FALSE(will_in_to_in_truncate(uint8_t{124}));
EXPECT_FALSE(will_in_to_in_truncate(0));
EXPECT_FALSE(will_in_to_in_truncate(-120));
}

TEST(WillConversionTruncate, AlwaysFalseByConventionForFloatingPointDestination) {
EXPECT_FALSE(will_conversion_truncate<float>(miles(18'000'000'000'000'000'000u), inches));
}

TEST(WillConversionTruncate, FloatToIntHandlesFractionalParts) {
EXPECT_TRUE(will_conversion_truncate<uint8_t>(feet(0.1), inches));
EXPECT_FALSE(will_conversion_truncate<uint8_t>(feet(1.0), inches));
}

TEST(IsConversionLossy, CorrectlyDiscriminatesBetweenLossyAndLosslessConversions) {
// We will check literally every representable value in the type, and make sure that the result
// of `is_conversion_lossy()` matches perfectly with the inability to recover the initial value.
Expand Down Expand Up @@ -943,6 +1000,16 @@ TEST(IsConversionLossy, CorrectlyDiscriminatesBetweenLossyAndLosslessConversions
test_round_trip_for_every_uint16_value(meters, yards);
}

TEST(IsConversionLossy, FloatToIntHandlesFractionalParts) {
EXPECT_TRUE(is_conversion_lossy<uint8_t>(feet(0.1), inches));
EXPECT_FALSE(is_conversion_lossy<uint8_t>(feet(1.0), inches));
}

TEST(IsConversionLossy, FloatToIntHandlesLimitsOfDestType) {
EXPECT_FALSE(is_conversion_lossy<uint8_t>(feet(21.0), inches));
EXPECT_TRUE(is_conversion_lossy<uint8_t>(feet(22.0), inches));
}

TEST(AreQuantityTypesEquivalent, RequiresSameRepAndEquivalentUnits) {
using IntQFeet = decltype(feet(1));
using IntQTwelveInches = decltype((inches * mag<12>())(1));
Expand Down
208 changes: 208 additions & 0 deletions au/code/au/static_cast_checkers.hh
Original file line number Diff line number Diff line change
@@ -0,0 +1,208 @@
// Copyright 2024 Aurora Operations, Inc.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

#pragma once

#include <cmath>
#include <exception>
#include <limits>
#include <type_traits>

namespace au {
namespace detail {

template <typename Source, typename Dest>
struct StaticCastChecker;

template <typename Dest, typename Source>
constexpr bool will_static_cast_overflow(Source x) {
return StaticCastChecker<Source, Dest>::will_static_cast_overflow(x);
}

template <typename Dest, typename Source>
constexpr bool will_static_cast_truncate(Source x) {
return StaticCastChecker<Source, Dest>::will_static_cast_truncate(x);
}

////////////////////////////////////////////////////////////////////////////////////////////////////
// Implementation details below.
////////////////////////////////////////////////////////////////////////////////////////////////////

////////////////////////////////////////////////////////////////////////////////////////////////////
// Overflow checking:

// Earlier enum values have higher priority than later ones.
enum class OverflowSituation {
DEST_BOUNDS_CONTAIN_SOURCE_BOUNDS,
UNSIGNED_TO_INTEGRAL,
SIGNED_TO_UNSIGNED,
SIGNED_TO_SIGNED,
FLOAT_TO_ANYTHING,

// If we categorize as this "catch-all" category, then we've hit a case that we haven't yet
// handled. This will result in a compiler error. We can decide how to handle it at that time.
UNEXPLORED,
};

template <typename Source, typename Dest>
constexpr OverflowSituation categorize_overflow_situation() {
static_assert(std::is_arithmetic<Source>::value && std::is_arithmetic<Dest>::value,
"Only arithmetic types are supported so far.");

if (std::is_integral<Source>::value && std::is_integral<Dest>::value) {
if ((std::is_signed<Source>::value == std::is_signed<Dest>::value) &&
(sizeof(Source) <= sizeof(Dest))) {
return OverflowSituation::DEST_BOUNDS_CONTAIN_SOURCE_BOUNDS;
}

if (std::is_unsigned<Source>::value) {
return OverflowSituation::UNSIGNED_TO_INTEGRAL;
}

return std::is_unsigned<Dest>::value ? OverflowSituation::SIGNED_TO_UNSIGNED
: OverflowSituation::SIGNED_TO_SIGNED;
}

if (std::is_integral<Source>::value && std::is_floating_point<Dest>::value) {
// For any integral-to-floating-point situation, `Dest` should always fully contain
// `Source`. This code simply double checks our assumption.
return ((static_cast<long double>(std::numeric_limits<Dest>::max()) >=
static_cast<long double>(std::numeric_limits<Source>::max())) &&
(static_cast<long double>(std::numeric_limits<Dest>::lowest()) <=
static_cast<long double>(std::numeric_limits<Source>::lowest())))
? OverflowSituation::DEST_BOUNDS_CONTAIN_SOURCE_BOUNDS
: OverflowSituation::UNEXPLORED;
}

if (std::is_floating_point<Source>::value && std::is_integral<Dest>::value) {
return OverflowSituation::FLOAT_TO_ANYTHING;
}

if (std::is_floating_point<Source>::value && std::is_floating_point<Dest>::value) {
return (sizeof(Source) <= sizeof(Dest))
? OverflowSituation::DEST_BOUNDS_CONTAIN_SOURCE_BOUNDS
: OverflowSituation::FLOAT_TO_ANYTHING;
}

return OverflowSituation::UNEXPLORED;
}

template <typename Source, typename Dest, OverflowSituation Cat>
struct StaticCastOverflowImpl;

template <typename Source, typename Dest>
struct StaticCastOverflowImpl<Source, Dest, OverflowSituation::DEST_BOUNDS_CONTAIN_SOURCE_BOUNDS> {
static constexpr bool will_static_cast_overflow(Source) { return false; }
};

template <typename Source, typename Dest>
struct StaticCastOverflowImpl<Source, Dest, OverflowSituation::UNSIGNED_TO_INTEGRAL> {
static constexpr bool will_static_cast_overflow(Source x) {
// Note that we know that the max value of `Dest` can fit into `Source`, because otherwise,
// this would have been categorized as `DEST_BOUNDS_CONTAIN_SOURCE_BOUNDS` rather than
// `UNSIGNED_TO_INTEGRAL`.
return x > static_cast<Source>(std::numeric_limits<Dest>::max());
}
};

template <typename Source, typename Dest>
struct StaticCastOverflowImpl<Source, Dest, OverflowSituation::SIGNED_TO_UNSIGNED> {
static constexpr bool will_static_cast_overflow(Source x) {
return (x < 0) ||
(static_cast<std::make_unsigned_t<Source>>(x) >
static_cast<std::make_unsigned_t<Source>>(std::numeric_limits<Dest>::max()));
}
};

template <typename Source, typename Dest>
struct StaticCastOverflowImpl<Source, Dest, OverflowSituation::SIGNED_TO_SIGNED> {
static constexpr bool will_static_cast_overflow(Source x) {
return (x < static_cast<Source>(std::numeric_limits<Dest>::lowest())) ||
(x > static_cast<Source>(std::numeric_limits<Dest>::max()));
}
};

template <typename Source, typename Dest>
struct StaticCastOverflowImpl<Source, Dest, OverflowSituation::FLOAT_TO_ANYTHING> {
static constexpr bool will_static_cast_overflow(Source x) {
// It's pretty safe to assume that `Source` can hold the limits of `Dest`, because otherwise
// this would have been categorized as `DEST_BOUNDS_CONTAIN_SOURCE_BOUNDS` rather than
// `FLOAT_TO_ANYTHING`.
return (x < static_cast<Source>(std::numeric_limits<Dest>::lowest())) ||
(x > static_cast<Source>(std::numeric_limits<Dest>::max()));
}
};

////////////////////////////////////////////////////////////////////////////////////////////////////
// Truncation checking:

enum class TruncationSituation {
CANNOT_TRUNCATE,
FLOAT_TO_INTEGRAL,

// If we categorize as this "catch-all" category, then we've hit a case that we haven't yet
// handled. This will result in a compiler error. We can decide how to handle it at that time.
UNEXPLORED,
};

template <typename Source, typename Dest>
constexpr TruncationSituation categorize_truncation_situation() {
static_assert(std::is_arithmetic<Source>::value && std::is_arithmetic<Dest>::value,
"Only arithmetic types are supported so far.");

if (std::is_same<Source, Dest>::value) {
return TruncationSituation::CANNOT_TRUNCATE;
}

if (std::is_floating_point<Dest>::value) {
// We explicitly treat floating point destinations as value-preserving, as does the rest of
// the library. This isn't strictly true, but if a user is going into the floating point
// domain, we assume they are OK with the usual floating point errors.
return TruncationSituation::CANNOT_TRUNCATE;
}

if (std::is_integral<Source>::value) {
return TruncationSituation::CANNOT_TRUNCATE;
}

if (std::is_floating_point<Source>::value && std::is_integral<Dest>::value) {
return TruncationSituation::FLOAT_TO_INTEGRAL;
}

return TruncationSituation::UNEXPLORED;
}

template <typename Source, typename Dest, TruncationSituation Cat>
struct StaticCastTruncateImpl;

template <typename Source, typename Dest>
struct StaticCastTruncateImpl<Source, Dest, TruncationSituation::CANNOT_TRUNCATE> {
static constexpr bool will_static_cast_truncate(Source) { return false; }
};

template <typename Source, typename Dest>
struct StaticCastTruncateImpl<Source, Dest, TruncationSituation::FLOAT_TO_INTEGRAL> {
static constexpr bool will_static_cast_truncate(Source x) { return std::trunc(x) != x; }
};

////////////////////////////////////////////////////////////////////////////////////////////////////
// Main implementation:

template <typename Source, typename Dest>
struct StaticCastChecker
: StaticCastOverflowImpl<Source, Dest, categorize_overflow_situation<Source, Dest>()>,
StaticCastTruncateImpl<Source, Dest, categorize_truncation_situation<Source, Dest>()> {};

} // namespace detail
} // namespace au
Loading

0 comments on commit 9aaeb79

Please sign in to comment.