Implemented more general version of ForwardDiff/RevereseDiff.

Implemented more general version of ForwardDiff/RevereseDiff.

In this CL:
  - Added more general version of ForwardDiff/RevereseDiff.
  - Replaced macro for testing if type T is of unsiged int with
    std::is_unsigned<T> instead.
  - Changed from EXPECT_XXX to ASSERT_XXX in the unittests since a lot of
    the test contains loops that would otherwise just spam the terminal.

BUG=

Committed: https://crrev.com/4c83c05b48d5f84520cdece992ac3fa5634a3c44
Cr-Commit-Position: refs/heads/master@{#11982}

[philipel, 2016-03-11 14:35:06]

philipel@webrtc.org changed reviewers:
+ terelius@webrtc.org, torbjorng@webrtc.org

[torbjorng (webrtc), 2016-03-11 14:54:22]

Looks good, but please consider my comments.

https://codereview.webrtc.org/1786043004/diff/1/webrtc/base/mod_ops.h
File webrtc/base/mod_ops.h (right):

https://codereview.webrtc.org/1786043004/diff/1/webrtc/base/mod_ops.h#newcode62
webrtc/base/mod_ops.h:62: template <typename T, T M>
Do we want these functions to work for e.g., T=uint8_t and M=256? The problem is
that M cannot be represented as T.

Either M needs to be stored in T' which in such corner cases needs to be made
larger, or perhaps more neatly we could truncate modulo as per the implicit
modulus of the type and make sure the core works for that (without upsetting
picky compilers).

I assume the intended use here is for compile-time constants M, so we could wrap
things conditions on M without cost.

https://codereview.webrtc.org/1786043004/diff/1/webrtc/base/mod_ops.h#newcode65
webrtc/base/mod_ops.h:65: "Type must be of unsigned integer.");
Please make more idiomatic, e.g., "Type must be an unsigned integer" or "Type
must of unsigned".

[philipel, 2016-03-11 16:04:38]

https://codereview.webrtc.org/1786043004/diff/1/webrtc/base/mod_ops.h
File webrtc/base/mod_ops.h (right):

https://codereview.webrtc.org/1786043004/diff/1/webrtc/base/mod_ops.h#newcode62
webrtc/base/mod_ops.h:62: template <typename T, T M>
On 2016/03/11 14:54:22, torbjorng (webrtc) wrote:
> Do we want these functions to work for e.g., T=uint8_t and M=256? The problem
is
> that M cannot be represented as T.
> 
> Either M needs to be stored in T' which in such corner cases needs to be made
> larger, or perhaps more neatly we could truncate modulo as per the implicit
> modulus of the type and make sure the core works for that (without upsetting
> picky compilers).
> 
> I assume the intended use here is for compile-time constants M, so we could
wrap
> things conditions on M without cost.

The other version is still available so you can do both:
ForwardDiff<uint8_t>(x, y)
ForwardDiff<uint8_t, 13>(x, y)

See line 71.

https://codereview.webrtc.org/1786043004/diff/1/webrtc/base/mod_ops.h#newcode65
webrtc/base/mod_ops.h:65: "Type must be of unsigned integer.");
On 2016/03/11 14:54:22, torbjorng (webrtc) wrote:
> Please make more idiomatic, e.g., "Type must be an unsigned integer" or "Type
> must of unsigned".

Done.

[torbjorng (webrtc), 2016-03-13 19:21:09]

lgtm

[terelius, 2016-03-13 22:59:15]

lgtm

[philipel, 2016-03-14 09:30:26]

The CQ bit was checked by philipel@webrtc.org

[philipel, 2016-03-14 09:30:30]

The CQ bit was unchecked by philipel@webrtc.org

[philipel, 2016-03-14 09:47:07]

philipel@webrtc.org changed reviewers:
+ mflodman@webrtc.org

[philipel, 2016-03-14 09:47:08]

Need owners approval for base/, mflodman@, ptal.

[mflodman, 2016-03-14 10:23:01]

lgtm

[philipel, 2016-03-14 10:28:54]

The CQ bit was checked by philipel@webrtc.org

[commit-bot: I haz the power, 2016-03-14 10:28:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1786043004/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1786043004/20001

[commit-bot: I haz the power, 2016-03-14 12:03:50]

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

[commit-bot: I haz the power, 2016-03-14 12:03:51]

Try jobs failed on following builders:
  android_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/11707)

[philipel, 2016-03-14 14:12:37]

The CQ bit was checked by philipel@webrtc.org

[commit-bot: I haz the power, 2016-03-14 14:12:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1786043004/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1786043004/20001

[commit-bot: I haz the power, 2016-03-14 15:43:38]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-03-14 15:43:48]

Description was changed from

==========
Implemented more general version of ForwardDiff/RevereseDiff.

In this CL:
  - Added more general version of ForwardDiff/RevereseDiff.
  - Replaced macro for testing if type T is of unsiged int with
    std::is_unsigned<T> instead.
  - Changed from EXPECT_XXX to ASSERT_XXX in the unittests since a lot of
    the test contains loops that would otherwise just spam the terminal.

BUG=
==========

to

==========
Implemented more general version of ForwardDiff/RevereseDiff.

In this CL:
  - Added more general version of ForwardDiff/RevereseDiff.
  - Replaced macro for testing if type T is of unsiged int with
    std::is_unsigned<T> instead.
  - Changed from EXPECT_XXX to ASSERT_XXX in the unittests since a lot of
    the test contains loops that would otherwise just spam the terminal.

BUG=

Committed: https://crrev.com/4c83c05b48d5f84520cdece992ac3fa5634a3c44
Cr-Commit-Position: refs/heads/master@{#11982}
==========

[commit-bot: I haz the power, 2016-03-14 15:43:49]

Patchset 2 (id:??) landed as
https://crrev.com/4c83c05b48d5f84520cdece992ac3fa5634a3c44
Cr-Commit-Position: refs/heads/master@{#11982}