Replace all use of the VERIFY macro.

Replace all use of the VERIFY macro.

Replaced by assigning value to a local variable, followed by a DCHECK.
Also deletes dead test code under the always false TEST_DIGEST define.

BUG=webrtc:6424
Review-Url: https://codereview.webrtc.org/2623473004
Cr-Commit-Position: refs/heads/master@{#16476}
Committed: https://chromium.googlesource.com/external/webrtc/+/c16fa5ea692e06992c30c8b9853356b90da74e2a

[nisse-webrtc, 2017-01-11 10:33:19]

Description was changed from

==========
Eliminate most use of base/common.h, and the ASSERT and VERIFY macros.

BUG=webrtc:6424
==========

to

==========
Replace all use of the ASSERT and VERIFY macros.

Most uses of ASSERT are replaced by RTC_DCHECK, by simple query
replace, with no attempt to switch to the right RTC_DCHECK_*
variant. ASSERT(false) is replaced by RTC_NOTREACHED(), and in
test-related files, RTC_CHECK is used rather than RTC_DCHECK.

Replacing the VERIFY macro is a little more involved, since side
effects must be preserved, and there were also a few occurances
of the pattern

  if (!VERIFY(...)) return false.

These are changed by assigning the checked value to a bool
variable, followed by an RTC_DCHECK.

Also eliminates most uses of the base/common.h include file.

BUG=webrtc:6424
==========

[nisse-webrtc, 2017-01-11 10:38:21]

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

[nisse-webrtc, 2017-01-11 10:38:22]

This monster patch replaces all use of ASSERT and VERIFY.

I hope I got it right, VERIFY is a little subtle, and the code got a bit uglier
in a few places.

Can we do this in one step, or would it make anything simpler of it is split in
smaller pieces?

As a follow up we can delete some or all of base/common.h, but I'd prefer to not
do that in the same cl, to limit interaction with webrtc users.

[kwiberg-webrtc, 2017-01-11 11:34:53]

On 2017/01/11 10:38:22, nisse-webrtc wrote:
> This monster patch replaces all use of ASSERT and VERIFY.
> 
> I hope I got it right, VERIFY is a little subtle, and the code got a
> bit uglier in a few places.
> 
> Can we do this in one step, or would it make anything simpler of it
> is split in smaller pieces?
> 
> As a follow up we can delete some or all of base/common.h, but I'd
> prefer to not do that in the same cl, to limit interaction with
> webrtc users.

Yes, please split it up! It'll make reviewing much easier. Points to consider:

  * You can split the patch by directory, but reviewing is even easier
    if it's split based on the type of change. (If the pieces are
    large enough, you may have to split by directory too.)

  * If some changes can be done fully automatically, with just a
    search+replace followed by clang-format, please do them in
    separate CLs. Machine-made CLs can be as large as you like, since
    they don't have to be reviewed for errors the same way
    hand-written CLs do. As soon as you start touching the code by
    hand, the reviewer has to look out for human error everywhere.

[nisse-webrtc, 2017-01-11 12:14:00]

On 2017/01/11 11:34:53, kwiberg-webrtc wrote:
> On 2017/01/11 10:38:22, nisse-webrtc wrote:
> > This monster patch replaces all use of ASSERT and VERIFY.
> > 
> > I hope I got it right, VERIFY is a little subtle, and the code got a
> > bit uglier in a few places.
> > 
> > Can we do this in one step, or would it make anything simpler of it
> > is split in smaller pieces?
> > 
> > As a follow up we can delete some or all of base/common.h, but I'd
> > prefer to not do that in the same cl, to limit interaction with
> > webrtc users.
> 
> Yes, please split it up! It'll make reviewing much easier. Points to consider:
> 
>   * You can split the patch by directory, but reviewing is even easier
>     if it's split based on the type of change. (If the pieces are
>     large enough, you may have to split by directory too.)
> 
>   * If some changes can be done fully automatically, with just a
>     search+replace followed by clang-format, please do them in
>     separate CLs. Machine-made CLs can be as large as you like, since
>     they don't have to be reviewed for errors the same way
>     hand-written CLs do. As soon as you start touching the code by
>     hand, the reviewer has to look out for human error everywhere.

First step now in cl https://codereview.webrtc.org/2625003003/. I'll keep this
cl
open, rebasing as parts of it are landed separately, to keep track as it
shrinks.

[nisse-webrtc, 2017-02-06 14:07:17]

Can you review the remaining changes, without splitting more parts out to
separate cls? Title and description would need to be revised before landing.

[kwiberg-webrtc, 2017-02-07 09:39:50]

https://codereview.webrtc.org/2623473004/diff/250001/webrtc/base/diskcache.cc
File webrtc/base/diskcache.cc (right):

https://codereview.webrtc.org/2623473004/diff/250001/webrtc/base/diskcache.cc...
webrtc/base/diskcache.cc:33: #endif
#define TRANSPARENT_CACHE_NAMES RTC_DCHECK_IS_ON

https://codereview.webrtc.org/2623473004/diff/250001/webrtc/base/network_unit...
File webrtc/base/network_unittest.cc (right):

https://codereview.webrtc.org/2623473004/diff/250001/webrtc/base/network_unit...
webrtc/base/network_unittest.cc:107: RTC_CHECK_EQ(list.size(), 1u);
Nit: Prefer to put the reference value first. Also, the "u" suffix isn't
necessary.

https://codereview.webrtc.org/2623473004/diff/250001/webrtc/base/physicalsock...
File webrtc/base/physicalsocketserver.cc (right):

https://codereview.webrtc.org/2623473004/diff/250001/webrtc/base/physicalsock...
webrtc/base/physicalsocketserver.cc:142: RTC_DCHECK (res == 0);
Spurious space. Also, RTC_DCHECK_EQ?

https://codereview.webrtc.org/2623473004/diff/250001/webrtc/base/physicalsock...
webrtc/base/physicalsocketserver.cc:842: }
Hmm. You're not supposed to handle DCHECK failures. So maybe do as in OnPreEvent
below?

https://codereview.webrtc.org/2623473004/diff/250001/webrtc/base/physicalsock...
webrtc/base/physicalsocketserver.cc:855: ssize_t res = read(afd_[0], b,
sizeof(b));
const? (You don't *have* to, but I recommend const wherever possible...)

https://codereview.webrtc.org/2623473004/diff/250001/webrtc/base/physicalsock...
webrtc/base/physicalsocketserver.cc:856: RTC_DCHECK(1 == res);
RTC_DCHECK_EQ, especially if you want the arguments in that order.

https://codereview.webrtc.org/2623473004/diff/250001/webrtc/base/socketadapte...
File webrtc/base/socketadapters.cc (right):

https://codereview.webrtc.org/2623473004/diff/250001/webrtc/base/socketadapte...
webrtc/base/socketadapters.cc:189: RTC_DCHECK(sizeof(kSslClientHello) == res);
RTC_DCHECK_EQ

https://codereview.webrtc.org/2623473004/diff/250001/webrtc/base/sslstreamada...
File webrtc/base/sslstreamadapter_unittest.cc (right):

https://codereview.webrtc.org/2623473004/diff/250001/webrtc/base/sslstreamada...
webrtc/base/sslstreamadapter_unittest.cc:380: RTC_CHECK_EQ(mtu_, 1460u);
Drop the "u".

https://codereview.webrtc.org/2623473004/diff/250001/webrtc/base/sslstreamada...
webrtc/base/sslstreamadapter_unittest.cc:419: RTC_CHECK_EQ(mtu_, 1460u);
Again, no "u".

https://codereview.webrtc.org/2623473004/diff/250001/webrtc/base/win32window.cc
File webrtc/base/win32window.cc (right):

https://codereview.webrtc.org/2623473004/diff/250001/webrtc/base/win32window....
webrtc/base/win32window.cc:68: RTC_DCHECK(res);
Or

  const XXX res = ::DestroyWindow(wnd_);
  RTC_DCHECK_NE(FALSE, res);

That's one less indirection in the boolean logic. Because I'm lazy, and I like
it when code doesn't require me to think too much...

https://codereview.webrtc.org/2623473004/diff/250001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvoe.h (right):

https://codereview.webrtc.org/2623473004/diff/250001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoe.h:32: // VERIFY, to ensure that there are no leaks
at shutdown
Stale comment.

https://codereview.webrtc.org/2623473004/diff/250001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/estimators/remb.cc (right):

https://codereview.webrtc.org/2623473004/diff/250001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/estimators/remb.cc:15: #include
"webrtc/base/common.h"  //  For RTC_UNUSED
Personal opinion: I don't think this sort of comment is useful---because they so
easily become incomplete or plain wrong, you can never rely on them anyway.

https://codereview.webrtc.org/2623473004/diff/250001/webrtc/p2p/base/turnport.cc
File webrtc/p2p/base/turnport.cc (right):

https://codereview.webrtc.org/2623473004/diff/250001/webrtc/p2p/base/turnport...
webrtc/p2p/base/turnport.cc:1481: RTC_DCHECK (res);
Spurious space again. Doesn't clang-format fix these?

[nisse-webrtc, 2017-02-07 10:48:00]

Description was changed from

==========
Replace all use of the ASSERT and VERIFY macros.

Most uses of ASSERT are replaced by RTC_DCHECK, by simple query
replace, with no attempt to switch to the right RTC_DCHECK_*
variant. ASSERT(false) is replaced by RTC_NOTREACHED(), and in
test-related files, RTC_CHECK is used rather than RTC_DCHECK.

Replacing the VERIFY macro is a little more involved, since side
effects must be preserved, and there were also a few occurances
of the pattern

  if (!VERIFY(...)) return false.

These are changed by assigning the checked value to a bool
variable, followed by an RTC_DCHECK.

Also eliminates most uses of the base/common.h include file.

BUG=webrtc:6424
==========

to

==========
Replace all use of the VERIFY macro.

Replaced by assigning value to a local variable, followed by a DCHECK.
Also deletes dead test code under the always false TEST_DIGEST define.

BUG=webrtc:6424
==========

[nisse-webrtc, 2017-02-07 10:48:16]

New better version.

https://codereview.webrtc.org/2623473004/diff/250001/webrtc/base/diskcache.cc
File webrtc/base/diskcache.cc (right):

https://codereview.webrtc.org/2623473004/diff/250001/webrtc/base/diskcache.cc...
webrtc/base/diskcache.cc:33: #endif
On 2017/02/07 09:39:49, kwiberg-webrtc wrote:
> #define TRANSPARENT_CACHE_NAMES RTC_DCHECK_IS_ON

Done.

https://codereview.webrtc.org/2623473004/diff/250001/webrtc/base/network_unit...
File webrtc/base/network_unittest.cc (right):

https://codereview.webrtc.org/2623473004/diff/250001/webrtc/base/network_unit...
webrtc/base/network_unittest.cc:107: RTC_CHECK_EQ(list.size(), 1u);
On 2017/02/07 09:39:49, kwiberg-webrtc wrote:
> Nit: Prefer to put the reference value first. Also, the "u" suffix isn't
> necessary.

Done.

https://codereview.webrtc.org/2623473004/diff/250001/webrtc/base/physicalsock...
File webrtc/base/physicalsocketserver.cc (right):

https://codereview.webrtc.org/2623473004/diff/250001/webrtc/base/physicalsock...
webrtc/base/physicalsocketserver.cc:842: }
On 2017/02/07 09:39:49, kwiberg-webrtc wrote:
> Hmm. You're not supposed to handle DCHECK failures. So maybe do as in
OnPreEvent
> below?

I'm deleting the if. And changing to const and DCHECK_EQ.

https://codereview.webrtc.org/2623473004/diff/250001/webrtc/base/physicalsock...
webrtc/base/physicalsocketserver.cc:855: ssize_t res = read(afd_[0], b,
sizeof(b));
On 2017/02/07 09:39:49, kwiberg-webrtc wrote:
> const? (You don't *have* to, but I recommend const wherever possible...)

Done.

https://codereview.webrtc.org/2623473004/diff/250001/webrtc/base/physicalsock...
webrtc/base/physicalsocketserver.cc:856: RTC_DCHECK(1 == res);
On 2017/02/07 09:39:49, kwiberg-webrtc wrote:
> RTC_DCHECK_EQ, especially if you want the arguments in that order.

Done.

https://codereview.webrtc.org/2623473004/diff/250001/webrtc/base/socketadapte...
File webrtc/base/socketadapters.cc (right):

https://codereview.webrtc.org/2623473004/diff/250001/webrtc/base/socketadapte...
webrtc/base/socketadapters.cc:189: RTC_DCHECK(sizeof(kSslClientHello) == res);
On 2017/02/07 09:39:49, kwiberg-webrtc wrote:
> RTC_DCHECK_EQ

Done.

https://codereview.webrtc.org/2623473004/diff/250001/webrtc/base/sslstreamada...
File webrtc/base/sslstreamadapter_unittest.cc (right):

https://codereview.webrtc.org/2623473004/diff/250001/webrtc/base/sslstreamada...
webrtc/base/sslstreamadapter_unittest.cc:380: RTC_CHECK_EQ(mtu_, 1460u);
On 2017/02/07 09:39:49, kwiberg-webrtc wrote:
> Drop the "u".

Done.

https://codereview.webrtc.org/2623473004/diff/250001/webrtc/base/sslstreamada...
webrtc/base/sslstreamadapter_unittest.cc:382: RTC_CHECK_EQ(lose_first_packet_,
0);
And these two are bools, not numbers. So changing to RTC_CHECK(!loss_) etc.

https://codereview.webrtc.org/2623473004/diff/250001/webrtc/base/sslstreamada...
webrtc/base/sslstreamadapter_unittest.cc:419: RTC_CHECK_EQ(mtu_, 1460u);
On 2017/02/07 09:39:49, kwiberg-webrtc wrote:
> Again, no "u".

Done.

https://codereview.webrtc.org/2623473004/diff/250001/webrtc/base/win32window.cc
File webrtc/base/win32window.cc (right):

https://codereview.webrtc.org/2623473004/diff/250001/webrtc/base/win32window....
webrtc/base/win32window.cc:68: RTC_DCHECK(res);
On 2017/02/07 09:39:49, kwiberg-webrtc wrote:
> Or
> 
>   const XXX res = ::DestroyWindow(wnd_);
>   RTC_DCHECK_NE(FALSE, res);
> 
> That's one less indirection in the boolean logic. Because I'm lazy, and I like
> it when code doesn't require me to think too much...

I think I'll instead write it as

const bool success = ::DestroyWindow(...);
RTC_DCHECK(success);

I think that's clearer than using the FALSE constant.

I know close to nothing about windows programming, but according to the docs
(https://msdn.microsoft.com/en-us/library/windows/desktop/ms632682(v=vs.85).aspx),
the return type is the windows type BOOL which seems to be numeric, and it's
documented to be non-zero on success and zero on failure. So then implicit
conversion to bool should do the right thing.

BTW, do you want me to rename other "res" variables to "success" too?

https://codereview.webrtc.org/2623473004/diff/250001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvoe.h (right):

https://codereview.webrtc.org/2623473004/diff/250001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoe.h:32: // VERIFY, to ensure that there are no leaks
at shutdown
On 2017/02/07 09:39:49, kwiberg-webrtc wrote:
> Stale comment.

Updated.

https://codereview.webrtc.org/2623473004/diff/250001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/estimators/remb.cc (right):

https://codereview.webrtc.org/2623473004/diff/250001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/estimators/remb.cc:15: #include
"webrtc/base/common.h"  //  For RTC_UNUSED
On 2017/02/07 09:39:49, kwiberg-webrtc wrote:
> Personal opinion: I don't think this sort of comment is useful---because they
so
> easily become incomplete or plain wrong, you can never rely on them anyway.

I'll delete this and similar comments. They were helpful to me while preparing
the cl, but I agree they're of queationable utility in the checked in code.

https://codereview.webrtc.org/2623473004/diff/250001/webrtc/p2p/base/dtlstran...
File webrtc/p2p/base/dtlstransportchannel.cc (right):

https://codereview.webrtc.org/2623473004/diff/250001/webrtc/p2p/base/dtlstran...
webrtc/p2p/base/dtlstransportchannel.cc:357: // TODO(nisse): Bad style of error
handling. Should it be a DCHECK or not?
And what to do about this case?

https://codereview.webrtc.org/2623473004/diff/250001/webrtc/p2p/base/turnport.cc
File webrtc/p2p/base/turnport.cc (right):

https://codereview.webrtc.org/2623473004/diff/250001/webrtc/p2p/base/turnport...
webrtc/p2p/base/turnport.cc:1481: RTC_DCHECK (res);
On 2017/02/07 09:39:49, kwiberg-webrtc wrote:
> Spurious space again. Doesn't clang-format fix these?

I'll run git cl format for the next patchset. I often hack on code following the
GNU coding standard, which prefers space before open paren...

[kwiberg-webrtc, 2017-02-07 11:45:08]

Much better.

https://codereview.webrtc.org/2623473004/diff/250001/webrtc/base/win32window.cc
File webrtc/base/win32window.cc (right):

https://codereview.webrtc.org/2623473004/diff/250001/webrtc/base/win32window....
webrtc/base/win32window.cc:68: RTC_DCHECK(res);
On 2017/02/07 10:48:15, nisse-webrtc wrote:
> On 2017/02/07 09:39:49, kwiberg-webrtc wrote:
> > Or
> > 
> >   const XXX res = ::DestroyWindow(wnd_);
> >   RTC_DCHECK_NE(FALSE, res);
> > 
> > That's one less indirection in the boolean logic. Because I'm lazy, and I
like
> > it when code doesn't require me to think too much...
> 
> I think I'll instead write it as
> 
> const bool success = ::DestroyWindow(...);
> RTC_DCHECK(success);
> 
> I think that's clearer than using the FALSE constant.
> 
> I know close to nothing about windows programming, but according to the docs
>
(https://msdn.microsoft.com/en-us/library/windows/desktop/ms632682(v=vs.85).aspx),
> the return type is the windows type BOOL which seems to be numeric, and it's
> documented to be non-zero on success and zero on failure. So then implicit
> conversion to bool should do the right thing.

Excellent.

> BTW, do you want me to rename other "res" variables to "success" too?

If it makes sense (i.e., if they're boolean, and true means successful), then
yes, that's probably a good idea. Some people are allergic to "res" variables,
but I'm not one of them.

https://codereview.webrtc.org/2623473004/diff/250001/webrtc/p2p/base/dtlstran...
File webrtc/p2p/base/dtlstransportchannel.cc (right):

https://codereview.webrtc.org/2623473004/diff/250001/webrtc/p2p/base/dtlstran...
webrtc/p2p/base/dtlstransportchannel.cc:357: // TODO(nisse): Bad style of error
handling. Should it be a DCHECK or not?
On 2017/02/07 10:48:15, nisse-webrtc wrote:
> And what to do about this case?

Dunno. The code should either handle the error, or it should never occur. Just
about the only thing we can be sure of is that our unit tests don't exercise the
error path, since that would have triggered the DCHECK...

Hmm. I just realized that that's a succinct way of explaining why you should
never handle DCHECK failures: the DCHECK works as a guard ensuring that the
error path is never tested...

But back to the immediate problem. I guess one sane way to treat this is to use
CHECK instead of DCHECK, and delete the error handling code, plus leaving a TODO
for yourself to replace the CHECK with a DCHECK in 3-6 months if it never
triggered.

[kwiberg-webrtc, 2017-02-07 11:46:12]

(By the way, do update the commit message too if you were going to. Ideally,
that should be part of the review too.)

[nisse-webrtc, 2017-02-07 13:52:40]

Resolved the dtls case by deleting the dcheck. And the title and description are
updated (mainly shortened).

https://codereview.webrtc.org/2623473004/diff/250001/webrtc/p2p/base/dtlstran...
File webrtc/p2p/base/dtlstransportchannel.cc (right):

https://codereview.webrtc.org/2623473004/diff/250001/webrtc/p2p/base/dtlstran...
webrtc/p2p/base/dtlstransportchannel.cc:357: // TODO(nisse): Bad style of error
handling. Should it be a DCHECK or not?
On 2017/02/07 11:45:08, kwiberg-webrtc wrote:
> On 2017/02/07 10:48:15, nisse-webrtc wrote:
> > And what to do about this case?
> 
> Dunno. The code should either handle the error, or it should never occur. Just
> about the only thing we can be sure of is that our unit tests don't exercise
the
> error path, since that would have triggered the DCHECK...
> 
> Hmm. I just realized that that's a succinct way of explaining why you should
> never handle DCHECK failures: the DCHECK works as a guard ensuring that the
> error path is never tested...
> 
> But back to the immediate problem. I guess one sane way to treat this is to
use
> CHECK instead of DCHECK, and delete the error handling code, plus leaving a
TODO
> for yourself to replace the CHECK with a DCHECK in 3-6 months if it never
> triggered.

After looking at the other code in this file, it seems other methods return
error when in an unexpected state, so I'm doing the same here and just delete
the DCHECK.

[kwiberg-webrtc, 2017-02-07 14:08:05]

lgtm

https://codereview.webrtc.org/2623473004/diff/310001/webrtc/pc/webrtcsdp.cc
File webrtc/pc/webrtcsdp.cc (right):

https://codereview.webrtc.org/2623473004/diff/310001/webrtc/pc/webrtcsdp.cc#n...
webrtc/pc/webrtcsdp.cc:1381: RTC_DCHECK(success);
:-)

[nisse-webrtc, 2017-02-07 14:09:31]

The CQ bit was checked by nisse@webrtc.org

[commit-bot: I haz the power, 2017-02-07 14:09:36]

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-02-07 15:18:44]

CQ is committing da patch.

Bot data: {"patchset_id": 310001, "attempt_start_ts": 1486476571028790,
"parent_rev": "ff0e72fd165facac27f0313aa178648782e63bc4", "commit_rev":
"c16fa5ea692e06992c30c8b9853356b90da74e2a"}

[commit-bot: I haz the power, 2017-02-07 15:18:46]

Description was changed from

==========
Replace all use of the VERIFY macro.

Replaced by assigning value to a local variable, followed by a DCHECK.
Also deletes dead test code under the always false TEST_DIGEST define.

BUG=webrtc:6424
==========

to

==========
Replace all use of the VERIFY macro.

Replaced by assigning value to a local variable, followed by a DCHECK.
Also deletes dead test code under the always false TEST_DIGEST define.

BUG=webrtc:6424

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

[commit-bot: I haz the power, 2017-02-07 15:18:47]

Committed patchset #17 (id:310001) as
https://chromium.googlesource.com/external/webrtc/+/c16fa5ea692e06992c30c8b98...