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

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

Specifically, they handle all combinations of two integer and two
floating-point arguments by picking a result type that is guaranteed
to be able to hold the result. This means callers no longer have to
deal with potentially dangerous casting to make all the arguments have
the same type, like they have to with std::min() and std::max().

Also, they're constexpr.

Mostly for illustrative purposes, this CL replaces a few std::min()
and std::max() calls with SafeMin() and SafeMax().

BUG=webrtc:7459
Review-Url: https://codereview.webrtc.org/2810483002
Cr-Commit-Position: refs/heads/master@{#17869}
Committed: https://chromium.googlesource.com/external/webrtc/+/7885d3f5c6bc6190bd4784cf41d42330ab6f9d6e

[kwiberg-webrtc, 2017-04-08 12:24:14]

The CQ bit was checked by kwiberg@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-08 12:24:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-04-08 12:59:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-08 12:59:31]

Dry run: Try jobs failed on following builders:
  linux_more_configs on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_more_configs/buil...)

[kwiberg-webrtc, 2017-04-09 11:48:37]

The CQ bit was checked by kwiberg@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-09 11:48:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-04-09 12:10:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-09 12:10:28]

Dry run: This issue passed the CQ dry run.

[kwiberg-webrtc, 2017-04-09 14:46:03]

kwiberg@webrtc.org changed reviewers:
+ ossu@webrtc.org

[aleloi, 2017-04-10 11:40:10]

I'm using this CL to learn template metaprogramming. Bear with me!

https://codereview.webrtc.org/2810483002/diff/20001/webrtc/base/safe_minmax.h
File webrtc/base/safe_minmax.h (right):

https://codereview.webrtc.org/2810483002/diff/20001/webrtc/base/safe_minmax.h...
webrtc/base/safe_minmax.h:63: bool all_int = IsIntlike<T1>::value&&
IsIntlike<T2>::value>
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.

https://codereview.webrtc.org/2810483002/diff/20001/webrtc/base/safe_minmax.h...
webrtc/base/safe_minmax.h:115: struct DefaultType;
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?

https://codereview.webrtc.org/2810483002/diff/20001/webrtc/base/safe_minmax.h...
webrtc/base/safe_minmax.h:132: typename T2 = safe_minmax_impl::DefaultType,
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.

https://codereview.webrtc.org/2810483002/diff/20001/webrtc/base/safe_minmax.h...
webrtc/base/safe_minmax.h:148: constexpr R2 SafeMax(T1 a, T2 b) {
Same comment as above.

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

This looks quite useful, but I'm concerned with the correctness when comparing
an integral and a floating-point type. Consider:

TEST(SafeMinMaxTest, TestEdgeCases) {
  const int64_t low = std::numeric_limits<int32_t>::lowest() + 1;
  const uint32_t high = std::numeric_limits<uint32_t>::max() - 1;
  const int32_t minv  = rtc::SafeMin(low, 1.0f);
  const uint32_t maxv = rtc::SafeMax(high, 1.0f);
  // SafeMin/Max deduces the return type to be float. Check explicitly as well.
  const int32_t minv2  = rtc::SafeMin<float>(low, 1.0f);
  const uint32_t maxv2 = rtc::SafeMax<float>(high, 1.0f);

  EXPECT_EQ(low,  minv);
  EXPECT_EQ(high, maxv);
  EXPECT_EQ(low,  minv2);
  EXPECT_EQ(high, maxv2);
}

I get (for both, but with different line numbers, of course):
../../webrtc/base/safe_minmax_unittest.cc:152: Failure
Value of: minv
  Actual: -2147483648
Expected: low
Which is: -2147483647
../../webrtc/base/safe_minmax_unittest.cc:153: Failure
Value of: maxv
  Actual: 0
Expected: high
Which is: 4294967294

If I constexpr (instead of const) all the values above, I get the following
compile time error (for maxv and maxv2)

../../webrtc/base/safe_minmax_unittest.cc:148:22: error: constexpr variable
'maxv' must be initialized by a constant expression
  constexpr uint32_t maxv = rtc::SafeMax(high, 1.0f);
                     ^      ~~~~~~~~~~~~~~~~~~~~~~~~
../../webrtc/base/safe_minmax_unittest.cc:148:29: note: value 4.2949673E+9 is
outside the range of representable values of type 'const uint32_t' (aka 'const
unsigned int')
  constexpr uint32_t maxv = rtc::SafeMax(high, 1.0f);

This will at least indicate that the user is doing something wrong, and get them
to investigate. Perhaps the former version is caught by a more stringent
compiler, like MSVC?

https://codereview.webrtc.org/2810483002/diff/20001/webrtc/base/safe_minmax.h
File webrtc/base/safe_minmax.h (right):

https://codereview.webrtc.org/2810483002/diff/20001/webrtc/base/safe_minmax.h...
webrtc/base/safe_minmax.h:63: bool all_int = IsIntlike<T1>::value&&
IsIntlike<T2>::value>
+Space before &&, it's not an rvalue-reference but a logical and, right?

[aleloi, 2017-04-10 14:08:12]

lowest int32_t should be -2**31=-2147483648;
low is then -2147483647, which is < 1.0f.

SafeMin(low, 1.0f) should produce a float, which probably can't represent 'low'
exactly. It's rounded to -2**31 and then implicitly converted to int32_t.

I'd say 'working as intended', unless we want to make SafeMin much more complex
(unsure how) to handle special cases like that.

[ossu, 2017-04-10 14:25:55]

On 2017/04/10 14:08:12, aleloi wrote:
> lowest int32_t should be -2**31=-2147483648;
> low is then -2147483647, which is < 1.0f.
> 
> SafeMin(low, 1.0f) should produce a float, which probably can't represent
'low'
> exactly. It's rounded to -2**31 and then implicitly converted to int32_t.
> 
> I'd say 'working as intended', unless we want to make SafeMin much more
complex
> (unsure how) to handle special cases like that.

Yes, that is what happens and I think that may be a problem. Since SafeMin/Max
obscures the actual types being used, it's important that that also doesn't
obscure programmer errors.
It's arguably there to make code safer, so it'd be unfortunate if, in some
cases, it didn't but instead added another set of rules the user must be aware
of.

For only integer types it seems water-proof and for only floating-point types as
well. I'd probably go so far as to consider the explicitly-typed version fine
too. If it claims to be safe, it needs to live up to that. :)

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

https://codereview.webrtc.org/2810483002/diff/20001/webrtc/base/safe_minmax.h
File webrtc/base/safe_minmax.h (right):

https://codereview.webrtc.org/2810483002/diff/20001/webrtc/base/safe_minmax.h...
webrtc/base/safe_minmax.h:63: bool all_int = IsIntlike<T1>::value&&
IsIntlike<T2>::value>
On 2017/04/10 11:40:09, aleloi wrote:
> 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.

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

https://codereview.webrtc.org/2810483002/diff/20001/webrtc/base/safe_minmax.h...
webrtc/base/safe_minmax.h:63: bool all_int = IsIntlike<T1>::value&&
IsIntlike<T2>::value>
On 2017/04/10 13:50:05, ossu wrote:
> +Space before &&, it's not an rvalue-reference but a logical and, right?

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

https://codereview.webrtc.org/2810483002/diff/20001/webrtc/base/safe_minmax.h...
webrtc/base/safe_minmax.h:115: struct DefaultType;
On 2017/04/10 11:40:09, aleloi wrote:
> Is this a common pattern?

Dunno. I haven't seen it before, I think.

> 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?

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.

https://codereview.webrtc.org/2810483002/diff/20001/webrtc/base/safe_minmax.h...
webrtc/base/safe_minmax.h:132: typename T2 = safe_minmax_impl::DefaultType,
On 2017/04/10 11:40:09, aleloi wrote:
> 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.

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.

[kwiberg-webrtc, 2017-04-12 18:38:54]

On 2017/04/10 14:25:55, ossu wrote:
> On 2017/04/10 14:08:12, aleloi wrote:
> > lowest int32_t should be -2**31=-2147483648;
> > low is then -2147483647, which is < 1.0f.
> > 
> > SafeMin(low, 1.0f) should produce a float, which probably can't represent
> 'low'
> > exactly. It's rounded to -2**31 and then implicitly converted to int32_t.
> > 
> > I'd say 'working as intended', unless we want to make SafeMin much more
> complex
> > (unsure how) to handle special cases like that.
> 
> Yes, that is what happens and I think that may be a problem. Since SafeMin/Max
> obscures the actual types being used, it's important that that also doesn't
> obscure programmer errors.
> It's arguably there to make code safer, so it'd be unfortunate if, in some
> cases, it didn't but instead added another set of rules the user must be aware
> of.
> 
> For only integer types it seems water-proof and for only floating-point types
as
> well. I'd probably go so far as to consider the explicitly-typed version fine
> too. If it claims to be safe, it needs to live up to that. :)

OK, I see what you mean about mixed integer/floating-point arguments---I'll
forbid the combination. Unfortunately, I'll have to forbid it even with an
explicitly specified return type, since the contract is that you can't screw
things up by giving a bad return type.

Some combinations would work---[u]int8_t and [u]int16_t can be converted to
float without loss, [u]int32_t can be converted to double, and int64_t can maybe
be converted to long double without loss---but that would be complex to support,
and hard to explain. So I'm not going to try.

[kwiberg-webrtc, 2017-04-12 23:10:24]

The CQ bit was checked by kwiberg@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-12 23:10:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-04-12 23:14:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-12 23:15:00]

Dry run: Try jobs failed on following builders:
  android_compile_x64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x64_dbg...)
  android_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/23309)

[kwiberg-webrtc, 2017-04-12 23:29:40]

The CQ bit was checked by kwiberg@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-12 23:29:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[kwiberg-webrtc, 2017-04-12 23:29:55]

Patchset #4 (id:60001) has been deleted

[kwiberg-webrtc, 2017-04-12 23:30:04]

Patchset #4 (id:80001) has been deleted

[kwiberg-webrtc, 2017-04-12 23:35:09]

Description was changed from

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

Specifically, they handle all combinations of integer and
floating-point arguments by picking a result type that is guaranteed
to be able to hold the result. This means callers no longer have to
deal with potentially dangerous casting to make all the arguments have
the same type, like they have to with std::min() and std::max().

Also, they're constexpr.

Mostly for illustrative purposes, this CL replaces a few std::min()
and std::max() calls with SafeMin() and SafeMax().

BUG=webrtc:7459
==========

to

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

Specifically, they handle all combinations of two integer and two
floating-point arguments by picking a result type that is guaranteed
to be able to hold the result. This means callers no longer have to
deal with potentially dangerous casting to make all the arguments have
the same type, like they have to with std::min() and std::max().

Also, they're constexpr.

Mostly for illustrative purposes, this CL replaces a few std::min()
and std::max() calls with SafeMin() and SafeMax().

BUG=webrtc:7459
==========

[commit-bot: I haz the power, 2017-04-13 00:08:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-13 00:08:14]

Dry run: This issue passed the CQ dry run.

[kwiberg-webrtc, 2017-04-13 00:56:05]

I think I managed to fix everything y'all complained about, except that I only
reported a bug for the bad && formatting instead of actually fixing it.

https://codereview.webrtc.org/2810483002/diff/100001/webrtc/base/safe_minmax.h
File webrtc/base/safe_minmax.h (right):

https://codereview.webrtc.org/2810483002/diff/100001/webrtc/base/safe_minmax....
webrtc/base/safe_minmax.h:156: typename safe_minmax_impl::UnderlyingType<
I also changed it so that the return type is the underlying type of an enum
type, instead of the enum type itself, since the returned value could easily be
one that's not listed in the enum.

[aleloi, 2017-04-13 09:31:31]

aleloi@webrtc.org changed reviewers:
+ aleloi@webrtc.org

[aleloi, 2017-04-13 09:31:32]

lgtm, I like it!

[kwiberg-webrtc, 2017-04-24 12:33:02]

ossu@: ping!

[ossu, 2017-04-24 13:22:28]

Sorry for the delay! I was meaning to patch this one in and test locally -
wanted to double-check that it threw up the appropriate errors. Unfortunately,
something doesn't seem 100% correct.

https://codereview.webrtc.org/2810483002/diff/100001/webrtc/base/safe_minmax_...
File webrtc/base/safe_minmax_unittest.cc (right):

https://codereview.webrtc.org/2810483002/diff/100001/webrtc/base/safe_minmax_...
webrtc/base/safe_minmax_unittest.cc:94: // static_assert(TypeCheckMaxR<uint64_t,
float, float>(), "");
I uncommented this line and the code still compiled just fine for me. With the
other commented-out lines, it failed though.

[kwiberg-webrtc, 2017-04-25 09:25:18]

The CQ bit was checked by kwiberg@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-25 09:25:31]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[kwiberg-webrtc, 2017-04-25 09:26:55]

https://codereview.webrtc.org/2810483002/diff/100001/webrtc/base/safe_minmax_...
File webrtc/base/safe_minmax_unittest.cc (right):

https://codereview.webrtc.org/2810483002/diff/100001/webrtc/base/safe_minmax_...
webrtc/base/safe_minmax_unittest.cc:94: // static_assert(TypeCheckMaxR<uint64_t,
float, float>(), "");
On 2017/04/24 13:22:27, ossu wrote:
> I uncommented this line and the code still compiled just fine for me. With the
> other commented-out lines, it failed though.

Thanks for checking! That error was supposed to be handled by a static_assert,
but apparently we don't ever actually reach that when we only inquire about what
type the result would have...

I uploaded a new patch set.

[commit-bot: I haz the power, 2017-04-25 09:46:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-25 09:46:06]

Dry run: This issue passed the CQ dry run.

[ossu, 2017-04-25 15:25:45]

On 2017/04/25 09:26:55, kwiberg-webrtc wrote:
>
https://codereview.webrtc.org/2810483002/diff/100001/webrtc/base/safe_minmax_...
> File webrtc/base/safe_minmax_unittest.cc (right):
> 
>
https://codereview.webrtc.org/2810483002/diff/100001/webrtc/base/safe_minmax_...
> webrtc/base/safe_minmax_unittest.cc:94: //
static_assert(TypeCheckMaxR<uint64_t,
> float, float>(), "");
> On 2017/04/24 13:22:27, ossu wrote:
> > I uncommented this line and the code still compiled just fine for me. With
the
> > other commented-out lines, it failed though.
> 
> Thanks for checking! That error was supposed to be handled by a static_assert,
> but apparently we don't ever actually reach that when we only inquire about
what
> type the result would have...
> 
> I uploaded a new patch set.

Tried it locally and tested a few other combinations of float and integer types.
Seems to work! LGTM!

[kwiberg-webrtc, 2017-04-25 19:32:04]

The CQ bit was checked by kwiberg@webrtc.org

[kwiberg-webrtc, 2017-04-25 19:32:05]

The patchset sent to the CQ was uploaded after l-g-t-m from aleloi@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2810483002/#ps120001 (title:
"trigger error earlier")

[commit-bot: I haz the power, 2017-04-25 19:32:15]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-04-25 19:35:08]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1493148724635340,
"parent_rev": "192c348aaf36ddce908f74bcb2161f467c0d8fa1", "commit_rev":
"7885d3f5c6bc6190bd4784cf41d42330ab6f9d6e"}

[commit-bot: I haz the power, 2017-04-25 19:35:12]

Description was changed from

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

Specifically, they handle all combinations of two integer and two
floating-point arguments by picking a result type that is guaranteed
to be able to hold the result. This means callers no longer have to
deal with potentially dangerous casting to make all the arguments have
the same type, like they have to with std::min() and std::max().

Also, they're constexpr.

Mostly for illustrative purposes, this CL replaces a few std::min()
and std::max() calls with SafeMin() and SafeMax().

BUG=webrtc:7459
==========

to

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

Specifically, they handle all combinations of two integer and two
floating-point arguments by picking a result type that is guaranteed
to be able to hold the result. This means callers no longer have to
deal with potentially dangerous casting to make all the arguments have
the same type, like they have to with std::min() and std::max().

Also, they're constexpr.

Mostly for illustrative purposes, this CL replaces a few std::min()
and std::max() calls with SafeMin() and SafeMax().

BUG=webrtc:7459

Review-Url: https://codereview.webrtc.org/2810483002
Cr-Commit-Position: refs/heads/master@{#17869}
Committed:
https://chromium.googlesource.com/external/webrtc/+/7885d3f5c6bc6190bd4784cf4...
==========

[commit-bot: I haz the power, 2017-04-25 19:35:13]

Committed patchset #5 (id:120001) as
https://chromium.googlesource.com/external/webrtc/+/7885d3f5c6bc6190bd4784cf4...