SafeMin/SafeMax: Fix wrong return type when given two enum arguments

SafeMin/SafeMax: Fix wrong return type when given two enum arguments

And add tests that catch it.

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

[kwiberg-webrtc, 2017-06-02 10:10:06]

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

[commit-bot: I haz the power, 2017-06-02 10:10:16]

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-06-02 10:14:03]

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

[kwiberg-webrtc, 2017-06-02 10:14:03]

I'm not exactly sure what went wrong, actually, but converting from enums to
real integer types earlier in the process seems like a good idea generally---it
makes things easier to reason about.

https://codereview.webrtc.org/2916083003/diff/1/webrtc/base/safe_minmax_unitt...
File webrtc/base/safe_minmax_unittest.cc (right):

https://codereview.webrtc.org/2916083003/diff/1/webrtc/base/safe_minmax_unitt...
webrtc/base/safe_minmax_unittest.cc:74: static_assert(TypeCheckMinMax<DefaultE, 
 UInt8E,      int,      int>(), "");
This (line 74) is the test that failed without the code change in this CL.

[commit-bot: I haz the power, 2017-06-02 10:37:41]

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

[commit-bot: I haz the power, 2017-06-02 10:37:42]

Dry run: This issue passed the CQ dry run.

[ossu, 2017-06-02 11:04:25]

On 2017/06/02 10:14:03, kwiberg-webrtc wrote:
> This (line 74) is the test that failed without the code change in this CL.

Added a check with an Int8-typed enum and that worked as well, but perhaps that
was never a problem?

diff --git a/webrtc/base/safe_minmax_unittest.cc
b/webrtc/base/safe_minmax_unittest.cc
index 519158b6e..351fa54f8 100644
--- a/webrtc/base/safe_minmax_unittest.cc
+++ b/webrtc/base/safe_minmax_unittest.cc
@@ -64,6 +64,7 @@ static_assert(TypeCheckMinMax<uint64_t, uint64_t, uint64_t,
uint64_t>(), "");
 
 enum DefaultE { kFoo = -17 };
 enum UInt8E : uint8_t { kBar = 17 };
+enum Int8E : int8_t { kBaz = -17 };
 
 // SafeMin/SafeMax: Check that we can use enum types.
 static_assert(TypeCheckMinMax<unsigned, unsigned, unsigned, unsigned>(), "");
@@ -75,6 +76,7 @@ static_assert(TypeCheckMinMax<DefaultE,   UInt8E,      int,   
  int>(), "");
 static_assert(TypeCheckMinMax<  UInt8E, unsigned,  uint8_t, unsigned>(), "");
 static_assert(TypeCheckMinMax<  UInt8E, DefaultE,     int,       int>(), "");
 static_assert(TypeCheckMinMax<  UInt8E,   UInt8E,  uint8_t,  uint8_t>(), "");
+static_assert(TypeCheckMinMax<  UInt8E,    Int8E,   int8_t,  uint8_t>(), "");

[ossu, 2017-06-02 11:05:33]

Also lgtm!

[kwiberg-webrtc, 2017-06-02 11:22:24]

The CQ bit was checked by kwiberg@webrtc.org

[commit-bot: I haz the power, 2017-06-02 11:22:30]

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-06-02 11:24:12]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1496402544415070, "parent_rev":
"2ff5b8347ae0e4f6b2b4275210902ca5a51f3133", "commit_rev":
"dbb497af84ada677a4efbba79c823762b274e570"}

[commit-bot: I haz the power, 2017-06-02 11:24:15]

Description was changed from

==========
SafeMin/SafeMax: Fix wrong return type when given two enum arguments

And add tests that catch it.

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

to

==========
SafeMin/SafeMax: Fix wrong return type when given two enum arguments

And add tests that catch it.

BUG=webrtc:7459

Review-Url: https://codereview.webrtc.org/2916083003
Cr-Commit-Position: refs/heads/master@{#18407}
Committed:
https://chromium.googlesource.com/external/webrtc/+/dbb497af84ada677a4efbba79...
==========

[commit-bot: I haz the power, 2017-06-02 11:24:16]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/external/webrtc/+/dbb497af84ada677a4efbba79...

[kwiberg-webrtc, 2017-06-02 11:24:48]

On 2017/06/02 11:04:25, ossu wrote:
> On 2017/06/02 10:14:03, kwiberg-webrtc wrote:
> > This (line 74) is the test that failed without the code change in this CL.
> 
> Added a check with an Int8-typed enum and that worked as well, but perhaps
that
> was never a problem?

I don't know---the testing has never been exhaustive (we couldn't write that
many tests by hand). Good to know that it works, though!