Improve RTC_DCHECK_op so that it won't trigger useless compiler warnings

Improve RTC_DCHECK_op so that it won't trigger useless compiler warnings

Before this change, with DCHECKs switched off, this sort of check

  size_t index = ...;
  RTC_DCHECK_GE(index, 0u);

would cause GCC (but no other compiler that we use) to complain
that unsigned values are always greater than or equal to 0. With
this change, it no longer complains.

(It was and remains the case that there was no complaints if
DCHECKs were switched on, or if you used RTC_CHECK_op.)

The reason for doing this change is that it isn't useful for the
compiler to force us to remove DCHECKs just because their
condition can be verified statically. That causes us to remove
the checks, and once that's happened, future code changes are free
to violate the removed checks and no one will know...

BUG=webrtc:6620
Committed: https://crrev.com/32bcaf61f52f604c7e268533b43f1ee0a61dff43
Cr-Commit-Position: refs/heads/master@{#14805}

[kwiberg-webrtc, 2016-10-27 10:54:06]

Description was changed from

==========
Improve RTC_DCHECK_op so that it won't trigger useless compiler warnings

Before this change, with DCHECKs switched off, this sort of check

  size_t index = ...;
  RTC_DCHECK_GE(index, 0u);

would cause GCC to complain that unsigned values are always greater
than or equal to 0. With this change, it no longer complains.

(It was and remains the case that there was no complaints if DCHECKs
were switched on, or if you used RTC_CHECK_op.)
==========

to

==========
Improve RTC_DCHECK_op so that it won't trigger useless compiler warnings

Before this change, with DCHECKs switched off, this sort of check

  size_t index = ...;
  RTC_DCHECK_GE(index, 0u);

would cause GCC (but no other compiler that we use) to complain
that unsigned values are always greater than or equal to 0. With this
change, it no longer complains.

(It was and remains the case that there was no complaints if DCHECKs
were switched on, or if you used RTC_CHECK_op.)
==========

[kwiberg-webrtc, 2016-10-27 10:57:50]

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

[kwiberg-webrtc, 2016-10-27 10:57:51]

Because I don't think it's useful for the compiler to force us to remove DCHECKs
just because their condition can be verified statically. That causes us to
remove the check, and once that's happened, future code changes are free to
violate the removed check and no one will know...

I'm also considering a follow-up that would make it so that you don't have to
write a "u" postfix to explicitly make an unsigned constant when comparing with
an unsigned variable...

[ossu, 2016-10-27 13:11:01]

I tried looking at the code and I'm not sure why this doesn't happen with
CHECKs, but I'm glad it doesn't. If this change stops us getting an error with
DCHECKs, then it LGTM. I added a comment as well.

https://codereview.webrtc.org/2455943002/diff/1/webrtc/base/checks.h
File webrtc/base/checks.h (right):

https://codereview.webrtc.org/2455943002/diff/1/webrtc/base/checks.h#newcode102
webrtc/base/checks.h:102: (void)(a), (void)(b), true))
Why ", true" at the end? Would you end up with a dual (void) (void) cast in
RTC_EAT_STREAM_PARAMETERS and that makes the compiler uncomfortable?

[kwiberg-webrtc, 2016-10-27 15:15:35]

Description was changed from

==========
Improve RTC_DCHECK_op so that it won't trigger useless compiler warnings

Before this change, with DCHECKs switched off, this sort of check

  size_t index = ...;
  RTC_DCHECK_GE(index, 0u);

would cause GCC (but no other compiler that we use) to complain
that unsigned values are always greater than or equal to 0. With this
change, it no longer complains.

(It was and remains the case that there was no complaints if DCHECKs
were switched on, or if you used RTC_CHECK_op.)
==========

to

==========
Improve RTC_DCHECK_op so that it won't trigger useless compiler warnings

Before this change, with DCHECKs switched off, this sort of check

  size_t index = ...;
  RTC_DCHECK_GE(index, 0u);

would cause GCC (but no other compiler that we use) to complain
that unsigned values are always greater than or equal to 0. With
this change, it no longer complains.

(It was and remains the case that there was no complaints if
DCHECKs were switched on, or if you used RTC_CHECK_op.)

The reason for doing this change is that it isn't useful for the
compiler to force us to remove DCHECKs just because their
condition can be verified statically. That causes us to remove
the checks, and once that's happened, future code changes are free
to violate the removed checks and no one will know...
==========

[kwiberg-webrtc, 2016-10-27 15:17:30]

On 2016/10/27 13:11:01, ossu wrote:
> I tried looking at the code and I'm not sure why this doesn't happen with
> CHECKs, but I'm glad it doesn't.

It's because in that case, we end up sending the rhs and lhs as separate
arguments to a function, and the compiler isn't smart enough to follow the
values through all the hoops. (Or maybe it chooses not to, on purpose, because
it would cause too many false positives.)

[kwiberg-webrtc, 2016-10-27 15:27:31]

https://codereview.webrtc.org/2455943002/diff/1/webrtc/base/checks.h
File webrtc/base/checks.h (right):

https://codereview.webrtc.org/2455943002/diff/1/webrtc/base/checks.h#newcode102
webrtc/base/checks.h:102: (void)(a), (void)(b), true))
On 2016/10/27 13:11:01, ossu wrote:
> Why ", true" at the end? Would you end up with a dual (void) (void) cast in
> RTC_EAT_STREAM_PARAMETERS and that makes the compiler uncomfortable?

Hmm. I wrote this before I changed RTC_EAT_STREAM_PARAMETERS (see above), so I
needed it to be convertible to bool, which wasn't the case for all types that b
has in practice. But I should be able to change that now, I guess.

[ossu, 2016-10-27 15:35:32]

On 2016/10/27 15:17:30, kwiberg-webrtc wrote:
> On 2016/10/27 13:11:01, ossu wrote:
> > I tried looking at the code and I'm not sure why this doesn't happen with
> > CHECKs, but I'm glad it doesn't.
> 
> It's because in that case, we end up sending the rhs and lhs as separate
> arguments to a function, and the compiler isn't smart enough to follow the
> values through all the hoops. (Or maybe it chooses not to, on purpose, because
> it would cause too many false positives.)

Ah, alright, that makes sense!

[kjellander_webrtc, 2016-10-27 19:56:39]

I know to little about macros to provide useful input here. I trust ossu!
Thanks for dealing with this. 

I guess I will discard https://codereview.webrtc.org/2450223002/ once this is
landed, if this fixes the Android build when dcheck_always_on=false?

[kwiberg-webrtc, 2016-10-27 20:21:48]

The CQ bit was checked by kwiberg@webrtc.org

[kwiberg-webrtc, 2016-10-27 20:21:49]

The patchset sent to the CQ was uploaded after l-g-t-m from ossu@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2455943002/#ps20001 (title:
"review comment")

[commit-bot: I haz the power, 2016-10-27 20:21:57]

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, 2016-10-27 20:24:06]

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

[commit-bot: I haz the power, 2016-10-27 20:24:07]

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

[kwiberg-webrtc, 2016-10-27 20:24:30]

On 2016/10/27 19:56:39, kjellander_webrtc wrote:
> I guess I will discard https://codereview.webrtc.org/2450223002/ once this is
> landed, if this fixes the Android build when dcheck_always_on=false?

Yes. I tested it locally with

  gn gen out/no-dcheck --args='use_goma=true is_debug=false
dcheck_always_on=false target_os="android" symbol_level=1'

This build failed before this CL, and succeeds afterwards.

[kwiberg-webrtc, 2016-10-27 20:34:30]

Description was changed from

==========
Improve RTC_DCHECK_op so that it won't trigger useless compiler warnings

Before this change, with DCHECKs switched off, this sort of check

  size_t index = ...;
  RTC_DCHECK_GE(index, 0u);

would cause GCC (but no other compiler that we use) to complain
that unsigned values are always greater than or equal to 0. With
this change, it no longer complains.

(It was and remains the case that there was no complaints if
DCHECKs were switched on, or if you used RTC_CHECK_op.)

The reason for doing this change is that it isn't useful for the
compiler to force us to remove DCHECKs just because their
condition can be verified statically. That causes us to remove
the checks, and once that's happened, future code changes are free
to violate the removed checks and no one will know...
==========

to

==========
Improve RTC_DCHECK_op so that it won't trigger useless compiler warnings

Before this change, with DCHECKs switched off, this sort of check

  size_t index = ...;
  RTC_DCHECK_GE(index, 0u);

would cause GCC (but no other compiler that we use) to complain
that unsigned values are always greater than or equal to 0. With
this change, it no longer complains.

(It was and remains the case that there was no complaints if
DCHECKs were switched on, or if you used RTC_CHECK_op.)

The reason for doing this change is that it isn't useful for the
compiler to force us to remove DCHECKs just because their
condition can be verified statically. That causes us to remove
the checks, and once that's happened, future code changes are free
to violate the removed checks and no one will know...

BUG=webrtc:6620
==========

[kwiberg-webrtc, 2016-10-27 20:34:41]

The CQ bit was checked by kwiberg@webrtc.org

[commit-bot: I haz the power, 2016-10-27 20:34:45]

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, 2016-10-27 20:36:01]

Description was changed from

==========
Improve RTC_DCHECK_op so that it won't trigger useless compiler warnings

Before this change, with DCHECKs switched off, this sort of check

  size_t index = ...;
  RTC_DCHECK_GE(index, 0u);

would cause GCC (but no other compiler that we use) to complain
that unsigned values are always greater than or equal to 0. With
this change, it no longer complains.

(It was and remains the case that there was no complaints if
DCHECKs were switched on, or if you used RTC_CHECK_op.)

The reason for doing this change is that it isn't useful for the
compiler to force us to remove DCHECKs just because their
condition can be verified statically. That causes us to remove
the checks, and once that's happened, future code changes are free
to violate the removed checks and no one will know...

BUG=webrtc:6620
==========

to

==========
Improve RTC_DCHECK_op so that it won't trigger useless compiler warnings

Before this change, with DCHECKs switched off, this sort of check

  size_t index = ...;
  RTC_DCHECK_GE(index, 0u);

would cause GCC (but no other compiler that we use) to complain
that unsigned values are always greater than or equal to 0. With
this change, it no longer complains.

(It was and remains the case that there was no complaints if
DCHECKs were switched on, or if you used RTC_CHECK_op.)

The reason for doing this change is that it isn't useful for the
compiler to force us to remove DCHECKs just because their
condition can be verified statically. That causes us to remove
the checks, and once that's happened, future code changes are free
to violate the removed checks and no one will know...

BUG=webrtc:6620
==========

[commit-bot: I haz the power, 2016-10-27 20:36:02]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-10-27 20:36:17]

Description was changed from

==========
Improve RTC_DCHECK_op so that it won't trigger useless compiler warnings

Before this change, with DCHECKs switched off, this sort of check

  size_t index = ...;
  RTC_DCHECK_GE(index, 0u);

would cause GCC (but no other compiler that we use) to complain
that unsigned values are always greater than or equal to 0. With
this change, it no longer complains.

(It was and remains the case that there was no complaints if
DCHECKs were switched on, or if you used RTC_CHECK_op.)

The reason for doing this change is that it isn't useful for the
compiler to force us to remove DCHECKs just because their
condition can be verified statically. That causes us to remove
the checks, and once that's happened, future code changes are free
to violate the removed checks and no one will know...

BUG=webrtc:6620
==========

to

==========
Improve RTC_DCHECK_op so that it won't trigger useless compiler warnings

Before this change, with DCHECKs switched off, this sort of check

  size_t index = ...;
  RTC_DCHECK_GE(index, 0u);

would cause GCC (but no other compiler that we use) to complain
that unsigned values are always greater than or equal to 0. With
this change, it no longer complains.

(It was and remains the case that there was no complaints if
DCHECKs were switched on, or if you used RTC_CHECK_op.)

The reason for doing this change is that it isn't useful for the
compiler to force us to remove DCHECKs just because their
condition can be verified statically. That causes us to remove
the checks, and once that's happened, future code changes are free
to violate the removed checks and no one will know...

BUG=webrtc:6620

Committed: https://crrev.com/32bcaf61f52f604c7e268533b43f1ee0a61dff43
Cr-Commit-Position: refs/heads/master@{#14805}
==========

[commit-bot: I haz the power, 2016-10-27 20:36:18]

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