Add SafeClamp(), which accepts args of different types

Add SafeClamp(), which accepts args of different types

Specifically, just like SafeMin() and SafeMax() it handles all
combinations of integer and all
combinations of floating-point arguments by picking a
result type that is guaranteed to be able to hold the result.

This CL also replaces a bunch of std::min + std:max call pairs with
calls to SafeClamp()---the ones that could easily be found by grep
because "min" and "max" were on the same line. :-)

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

[kwiberg-webrtc, 2017-04-08 12:29:47]

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:29:53]

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 14:30:14]

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

[commit-bot: I haz the power, 2017-04-08 14:30:16]

Dry run: Try jobs failed on following builders:
  linux_baremetal on master.tryserver.webrtc (JOB_TIMED_OUT, build hasn't
started yet, builder probably lacks capacity)
  mac_baremetal on master.tryserver.webrtc (JOB_TIMED_OUT, build hasn't started
yet, builder probably lacks capacity)
  win_baremetal on master.tryserver.webrtc (JOB_TIMED_OUT, build hasn't started
yet, builder probably lacks capacity)

[kwiberg-webrtc, 2017-04-09 11:53:15]

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:53:23]

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:23:06]

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

[commit-bot: I haz the power, 2017-04-09 12:23:07]

Dry run: This issue passed the CQ dry run.

[kwiberg-webrtc, 2017-06-02 09:33:16]

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

[commit-bot: I haz the power, 2017-06-02 09:33:18]

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-06-02 10:18:25]

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

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

Dry run: This issue passed the CQ dry run.

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

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

[commit-bot: I haz the power, 2017-06-02 11:28:18]

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 11:29:57]

Description was changed from

==========
Add SafeClamp(), which accepts args of different types

Specifically, just like SafeMin() and SafeMax() it handles all
combinations of integer and floating-point arguments by picking a
result type that is guaranteed to be able to hold the result.

This CL also replaces a bunch of std::min + std:max call pairs with
calls to SafeClamp()---the ones that could easily be found by grep
because "min" and "max" were on the same line. :-)

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

to

==========
Add SafeClamp(), which accepts args of different types

Specifically, just like SafeMin() and SafeMax() it handles all
combinations of integer and all
combinations of floating-point arguments by picking a
result type that is guaranteed to be able to hold the result.

This CL also replaces a bunch of std::min + std:max call pairs with
calls to SafeClamp()---the ones that could easily be found by grep
because "min" and "max" were on the same line. :-)

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

[commit-bot: I haz the power, 2017-06-02 11:57:17]

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

[commit-bot: I haz the power, 2017-06-02 11:57:18]

Dry run: This issue passed the CQ dry run.

[kwiberg-webrtc, 2017-06-02 12:39:58]

Patchset #1 (id:1) has been deleted

[kwiberg-webrtc, 2017-06-02 12:40:12]

Patchset #1 (id:20001) has been deleted

[kwiberg-webrtc, 2017-06-02 12:40:27]

Patchset #1 (id:40001) has been deleted

[kwiberg-webrtc, 2017-06-02 12:47:48]

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

[nisse-webrtc, 2017-06-02 13:00:54]

https://codereview.webrtc.org/2808513003/diff/60001/webrtc/base/safe_minmax.h
File webrtc/base/safe_minmax.h (right):

https://codereview.webrtc.org/2808513003/diff/60001/webrtc/base/safe_minmax.h...
webrtc/base/safe_minmax.h:31: //   rtc::SafeClamp(a, b, x)
Do you have a good reason for this argument order? I'd put x first, and read it
"clamp x to the [a,b] interval".

[kwiberg-webrtc, 2017-06-02 13:12:37]

https://codereview.webrtc.org/2808513003/diff/60001/webrtc/base/safe_minmax.h
File webrtc/base/safe_minmax.h (right):

https://codereview.webrtc.org/2808513003/diff/60001/webrtc/base/safe_minmax.h...
webrtc/base/safe_minmax.h:31: //   rtc::SafeClamp(a, b, x)
On 2017/06/02 13:00:54, nisse-webrtc wrote:
> Do you have a good reason for this argument order? I'd put x first, and read
it
> "clamp x to the [a,b] interval".

Yes---at least in my mind, the a and b arguments are "less variable", so it
feels right to put them closer to the function name.

In a language like Haskell, currying makes this point clear:

  SafeClamp a b x  -- clamp x to [a, b]
  SafeClamp a b    -- function that clamps its argument to [a, b]

In C++, there's no technical reason for this (or any other) argument order, but
it still feels right to order the arguments this way. In other words, I don't
think of SafeClamp(a, b, x) as "clamp x to [a, b]", I think of it as "apply
'clamp to [a, b]' to x".

[ossu, 2017-06-02 13:22:34]

https://codereview.webrtc.org/2808513003/diff/60001/webrtc/base/safe_minmax.h
File webrtc/base/safe_minmax.h (right):

https://codereview.webrtc.org/2808513003/diff/60001/webrtc/base/safe_minmax.h...
webrtc/base/safe_minmax.h:31: //   rtc::SafeClamp(a, b, x)
On 2017/06/02 13:00:54, nisse-webrtc wrote:
> Do you have a good reason for this argument order? I'd put x first, and read
it
> "clamp x to the [a,b] interval".

I've got to agree with nisse here, in my mind having the value to be clamped
first makes the most sense. Fortunately, we don't have to rely completely on my
mind here. Looking at C++17's std::clamp, it's
std::clamp(v, lo, hi), (see: http://en.cppreference.com/w/cpp/algorithm/clamp).

I think it makes sense to follow that order, since SafeClamp will generally be a
stand-in for std::clamp, with some extra guarantees.

[kwiberg-webrtc, 2017-06-04 01:17:59]

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

[commit-bot: I haz the power, 2017-06-04 01:18:04]

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-04 01:23:50]

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

[commit-bot: I haz the power, 2017-06-04 01:23:59]

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-04 01:24:05]

Patchset #2 (id:80001) has been deleted

[kwiberg-webrtc, 2017-06-04 01:27:54]

https://codereview.webrtc.org/2808513003/diff/60001/webrtc/base/safe_minmax.h
File webrtc/base/safe_minmax.h (right):

https://codereview.webrtc.org/2808513003/diff/60001/webrtc/base/safe_minmax.h...
webrtc/base/safe_minmax.h:31: //   rtc::SafeClamp(a, b, x)
On 2017/06/02 13:22:33, ossu wrote:
> On 2017/06/02 13:00:54, nisse-webrtc wrote:
> > Do you have a good reason for this argument order? I'd put x first, and read
> it
> > "clamp x to the [a,b] interval".
> 
> I've got to agree with nisse here, in my mind having the value to be clamped
> first makes the most sense. Fortunately, we don't have to rely completely on
my
> mind here. Looking at C++17's std::clamp, it's
> std::clamp(v, lo, hi), (see:
http://en.cppreference.com/w/cpp/algorithm/clamp).
> 
> I think it makes sense to follow that order, since SafeClamp will generally be
a
> stand-in for std::clamp, with some extra guarantees.

OK, fine. You all have terrible taste, but there are lots more of you than of
me. ;-)

New patch set uploaded that changes the argument order, and makes some minor
changes to the comments in this file.

[commit-bot: I haz the power, 2017-06-04 01:51:03]

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

[commit-bot: I haz the power, 2017-06-04 01:51:04]

Dry run: This issue passed the CQ dry run.

[nisse-webrtc, 2017-06-08 06:46:45]

lgtm (but I don't really follow the C++ templating magic).

[kwiberg-webrtc, 2017-06-08 08:37:36]

On 2017/06/08 06:46:45, nisse-webrtc wrote:
> lgtm (but I don't really follow the C++ templating magic).

If you're interested, I can come over and explain it to you.

ossu@, I'll wait for your verdict as well (but no hurries).

[ossu, 2017-06-12 12:21:36]

Couple of tiny nits, but otherwise LGTM.

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

https://codereview.webrtc.org/2808513003/diff/100001/webrtc/base/safe_minmax....
webrtc/base/safe_minmax.h:296: // Pick the best type, preferring the same
signedness at T but falling back
nit: "the same signedness _as_ T"

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

https://codereview.webrtc.org/2808513003/diff/100001/webrtc/base/safe_minmax_...
webrtc/base/safe_minmax_unittest.cc:314: int32_t TestClampRef(int32_t x, int32_t
a, int32_t b) {
What are these (TestClampRef/TestClampSafe)for? I can't find they're ever
called. Maybe TestClampSafe are there to ensure those calls don't result in
static_asserts failing, but the Ref ones?

https://codereview.webrtc.org/2808513003/diff/100001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/adaptive_fir_filter_unittest.cc
(right):

https://codereview.webrtc.org/2808513003/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter_unittest.cc:355: a =
rtc::SafeClamp<float>(a, -32768.f, 32767.f);
Why does this one need an explicit return type? Looks to me three floats in
should lead to a float out.

[kwiberg-webrtc, 2017-06-12 17:27:35]

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

[commit-bot: I haz the power, 2017-06-12 17:27:40]

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-12 17:28:12]

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

https://codereview.webrtc.org/2808513003/diff/100001/webrtc/base/safe_minmax....
webrtc/base/safe_minmax.h:296: // Pick the best type, preferring the same
signedness at T but falling back
On 2017/06/12 12:21:35, ossu wrote:
> nit: "the same signedness _as_ T"

Done.

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

https://codereview.webrtc.org/2808513003/diff/100001/webrtc/base/safe_minmax_...
webrtc/base/safe_minmax_unittest.cc:314: int32_t TestClampRef(int32_t x, int32_t
a, int32_t b) {
On 2017/06/12 12:21:36, ossu wrote:
> What are these (TestClampRef/TestClampSafe)for? I can't find they're ever
> called. Maybe TestClampSafe are there to ensure those calls don't result in
> static_asserts failing, but the Ref ones?

See the comment on lines 300-304. I'll try to rearrange it to make it more
obvious that the comment applies here too.

https://codereview.webrtc.org/2808513003/diff/100001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/adaptive_fir_filter_unittest.cc
(right):

https://codereview.webrtc.org/2808513003/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter_unittest.cc:355: a =
rtc::SafeClamp<float>(a, -32768.f, 32767.f);
On 2017/06/12 12:21:36, ossu wrote:
> Why does this one need an explicit return type? Looks to me three floats in
> should lead to a float out.

Done.

[commit-bot: I haz the power, 2017-06-12 17:47:46]

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

[commit-bot: I haz the power, 2017-06-12 17:47:47]

Dry run: This issue passed the CQ dry run.

[kwiberg-webrtc, 2017-06-12 18:38:33]

The CQ bit was checked by kwiberg@webrtc.org

[kwiberg-webrtc, 2017-06-12 18:38:34]

The patchset sent to the CQ was uploaded after l-g-t-m from nisse@webrtc.org,
ossu@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2808513003/#ps140001 (title:
"rebase")

[commit-bot: I haz the power, 2017-06-12 18:38:43]

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-12 18:40:48]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1497292713609210,
"parent_rev": "d1114c7fefd8faf087a2bf8dcc91c5bdc31def5c", "commit_rev":
"0703856b53e80159d666e289762406989f08ccfa"}

[commit-bot: I haz the power, 2017-06-12 18:40:54]

Description was changed from

==========
Add SafeClamp(), which accepts args of different types

Specifically, just like SafeMin() and SafeMax() it handles all
combinations of integer and all
combinations of floating-point arguments by picking a
result type that is guaranteed to be able to hold the result.

This CL also replaces a bunch of std::min + std:max call pairs with
calls to SafeClamp()---the ones that could easily be found by grep
because "min" and "max" were on the same line. :-)

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

to

==========
Add SafeClamp(), which accepts args of different types

Specifically, just like SafeMin() and SafeMax() it handles all
combinations of integer and all
combinations of floating-point arguments by picking a
result type that is guaranteed to be able to hold the result.

This CL also replaces a bunch of std::min + std:max call pairs with
calls to SafeClamp()---the ones that could easily be found by grep
because "min" and "max" were on the same line. :-)

BUG=webrtc:7459

Review-Url: https://codereview.webrtc.org/2808513003
Cr-Commit-Position: refs/heads/master@{#18542}
Committed:
https://chromium.googlesource.com/external/webrtc/+/0703856b53e80159d666e2897...
==========

[commit-bot: I haz the power, 2017-06-12 18:40:55]

Committed patchset #4 (id:140001) as
https://chromium.googlesource.com/external/webrtc/+/0703856b53e80159d666e2897...