Add SafeMin() and SafeMax(), which accept args of different types

webrtc/base/safe_minmax.h

+/*
+ *  Copyright 2017 The WebRTC Project Authors. All rights reserved.
+ *
+ *  Use of this source code is governed by a BSD-style license
+ *  that can be found in the LICENSE file in the root of the source
+ *  tree. An additional intellectual property rights grant can be found
+ *  in the file PATENTS.  All contributing project authors may
+ *  be found in the AUTHORS file in the root of the source tree.
+ */
+
+// Minimum and maximum
+// ===================
+//
+//   rtc::SafeMin(x, y)
+//   rtc::SafeMax(x, y)
+//
+// Accept two arguments of any mix of integral and floating-point types, and
+// return the smaller and larger value, respectively, with no truncation or
+// wrap-around. If only one of the input types is statically guaranteed to be
+// able to represent the result, the return type is that type; if either one
+// would do, the result type is the smaller type. (One of these two cases
+// always applies.)
+//
+// Requesting a specific return type
+// =================================
+//
+// Both functions allow callers to explicitly specify the return type as a
+// template parameter, overriding the default return type. E.g.
+//
+//   rtc::SafeMin<int>(x, y)  // returns an int
+//
+// If the requested type is statically guaranteed to be able to represent the
+// result, then everything's fine, and the return type is as requested. But if
+// the requested type is too small, a static_assert is triggered.
+
+#ifndef WEBRTC_BASE_SAFE_MINMAX_H_
+#define WEBRTC_BASE_SAFE_MINMAX_H_
+
+#include <limits>
+#include <type_traits>
+
+#include "webrtc/base/checks.h"
+#include "webrtc/base/safe_compare.h"
+#include "webrtc/base/type_traits.h"
+
+namespace rtc {
+
+namespace safe_minmax_impl {
+
+// Make the range of a type available via something other than a constexpr
+// function, to work around MSVC limitations. See
+// https://blogs.msdn.microsoft.com/vcblog/2015/12/02/partial-support-for-expression-sfinae-in-vs-2015-update-1/
+template <typename T>
+struct Limits {
+  static constexpr T lowest = std::numeric_limits<T>::lowest();
+  static constexpr T max = std::numeric_limits<T>::max();
+};
+
+// Given two types T1 and T2, find types that can hold the smallest (in
+// ::min_t) and the largest (in ::max_t) of the two values.
+template <typename T1,
+          typename T2,
+          bool all_int = IsIntlike<T1>::value&& IsIntlike<T2>::value>

[aleloi, 2017/04/10 11:40:09]

IsIntLike seems to also handle int-based enums. Is it possible to add a unit
test for SafeMin on enum types? Or perhaps change to std::is_integral.

[ossu, 2017/04/10 13:50:05]

+Space before &&, it's not an rvalue-reference but a logical and, right?

[kwiberg-webrtc, 2017/04/12 18:21:20]

Yes, a unit test with enum values would be good. Will do.

[kwiberg-webrtc, 2017/04/12 18:21:21]

I agree. Unfortunately, clang-format does not. OK if I throw a bug at them and
just wait for them to fix it?

+struct MType;
+
+// Specialization for when at least one of the types is floating-point.
+template <typename T1, typename T2>
+struct MType<T1, T2, false> {
+  using min_t = typename std::common_type<T1, T2>::type;
+  static_assert(std::is_same<min_t, T1>::value ||
+                    std::is_same<min_t, T2>::value,
+                "");
+
+  using max_t = typename std::common_type<T1, T2>::type;
+  static_assert(std::is_same<max_t, T1>::value ||
+                    std::is_same<max_t, T2>::value,
+                "");
+};
+
+// Specialization for when both types are integral.
+template <typename T1, typename T2>
+struct MType<T1, T2, true> {
+  // The type with the lowest minimum value. In case of a tie, the type with
+  // the lowest maximum value. In case that too is a tie, the types have the
+  // same range, and we arbitrarily pick T1.
+  using min_t = typename std::conditional<
+      safe_cmp::Lt(Limits<T1>::lowest, Limits<T2>::lowest),
+      T1,
+      typename std::conditional<
+          safe_cmp::Gt(Limits<T1>::lowest, Limits<T2>::lowest),
+          T2,
+          typename std::conditional<safe_cmp::Le(Limits<T1>::max,
+                                                 Limits<T2>::max),
+                                    T1,
+                                    T2>::type>::type>::type;
+  static_assert(std::is_same<min_t, T1>::value ||
+                    std::is_same<min_t, T2>::value,
+                "");
+
+  // The type with the highest maximum value. In case of a tie, the types have
+  // the same range (because in C++, integer types with the same maximum also
+  // have the same minimum).
+  static_assert(safe_cmp::Ne(Limits<T1>::max, Limits<T2>::max) ||
+                    safe_cmp::Eq(Limits<T1>::lowest, Limits<T2>::lowest),
+                "integer types with the same max should have the same min");
+  using max_t = typename std::
+      conditional<safe_cmp::Ge(Limits<T1>::max, Limits<T2>::max), T1, T2>::type;
+  static_assert(std::is_same<max_t, T1>::value ||
+                    std::is_same<max_t, T2>::value,
+                "");
+};
+
+// A dummy type that we pass around at compile time but never actually use.
+// Declared but not defined.
+struct DefaultType;

[aleloi, 2017/04/10 11:40:09]

Is this a common pattern? Is there something that can be used instead of
DefaultType in the standard library, preferably a type with no inhabitant? Can
'void' be used instead of DefaultType?

[kwiberg-webrtc, 2017/04/12 18:21:20]

Dunno. I haven't seen it before, I think.
Potentially yes, but this construction is safer the more unlikely users are to
actually be using the sentinel type. So making a new, local sentinel type seems
like the safest choice.

+
+// ::type is A, except we fall back to B if A is DefaultType. We static_assert
+// that the chosen type can hold all values that B can hold.
+template <typename A, typename B>
+struct TypeOr {
+  using type = typename std::
+      conditional<std::is_same<A, DefaultType>::value, B, A>::type;
+  static_assert(safe_cmp::Le(Limits<type>::lowest, Limits<B>::lowest) &&
+                    safe_cmp::Ge(Limits<type>::max, Limits<B>::max),
+                "The specified type isn't large enough");
+};
+
+}  // namespace safe_minmax_impl
+
+template <typename R = safe_minmax_impl::DefaultType,
+          typename T1 = safe_minmax_impl::DefaultType,
+          typename T2 = safe_minmax_impl::DefaultType,

[aleloi, 2017/04/10 11:40:09]

I wonder if the function signature can be made simpler. Typically, SafeMin will
be used with no or a single template parameter. In the unit test, only R is ever
specified. To make it simpler to understand and use, I'd like to change it to 

template<typename R = ..., typename T1, typename T2>
 [return type] safeMin(T1 a, T2 b);

Would it be possible to say

[return type] = 
  TypeOr<R, typename safe_minmax_impl::MType<T1, T2>::min_t>::type>

or hide it in a struct template

[return type] =
 safe_minmax_impl::GetMinType<R, T1, T2>::type


Or maybe that only makes it worse.

[kwiberg-webrtc, 2017/04/12 18:21:21]

The only other good-ish alternative I see is to not have R2 as a template
parameter, but instead repeat the expression twice---once in the return type,
and one in a function-local type alias. But it's complex enough that I really
like not having to repeat it.

+          typename R2 = typename safe_minmax_impl::
+              TypeOr<R, typename safe_minmax_impl::MType<T1, T2>::min_t>::type>
+constexpr R2 SafeMin(T1 a, T2 b) {
+  static_assert(IsIntlike<T1>::value || std::is_floating_point<T1>::value,
+                "The first argument must be integral or floating-point");
+  static_assert(IsIntlike<T2>::value || std::is_floating_point<T2>::value,
+                "The second argument must be integral or floating-point");
+  return safe_cmp::Lt(a, b) ? static_cast<R2>(a) : static_cast<R2>(b);
+}
+
+template <typename R = safe_minmax_impl::DefaultType,
+          typename T1 = safe_minmax_impl::DefaultType,
+          typename T2 = safe_minmax_impl::DefaultType,
+          typename R2 = typename safe_minmax_impl::
+              TypeOr<R, typename safe_minmax_impl::MType<T1, T2>::max_t>::type>
+constexpr R2 SafeMax(T1 a, T2 b) {

[aleloi, 2017/04/10 11:40:09]

Same comment as above.

+  static_assert(IsIntlike<T1>::value || std::is_floating_point<T1>::value,
+                "The first argument must be integral or floating-point");
+  static_assert(IsIntlike<T2>::value || std::is_floating_point<T2>::value,
+                "The second argument must be integral or floating-point");
+  return safe_cmp::Gt(a, b) ? static_cast<R2>(a) : static_cast<R2>(b);
+}
+
+}  // namespace rtc
+
+#endif  // WEBRTC_BASE_SAFE_MINMAX_H_