Test RTC_DCHECK_IS_ON instead of checking DCHECK_ALWAYS_ON everywhere

Test RTC_DCHECK_IS_ON instead of checking DCHECK_ALWAYS_ON everywhere

The former is always defined (by webrtc/base/checks.h) to either 0 or
1, whereas the latter isn't necessarily defined.

NOTRY=true
BUG=webrtc:6451
Committed: https://crrev.com/ab0b929321d37669165d5795268fa10a8c97ec5b
Cr-Commit-Position: refs/heads/master@{#14474}

[kwiberg-webrtc, 2016-09-30 08:55:10]

Description was changed from

==========
Test RTC_DCHECK_IS_ON instead of checking DCHECK_ALWAYS_ON everywhere

The former is always defined (by webrtc/base/checks.h) to either 0 or
1, whereas the latter isn't necessarily defined.

BUG=webrtc:6398
==========

to

==========
Test RTC_DCHECK_IS_ON instead of checking DCHECK_ALWAYS_ON everywhere

The former is always defined (by webrtc/base/checks.h) to either 0 or
1, whereas the latter isn't necessarily defined.

BUG=webrtc:6451
==========

[kwiberg-webrtc, 2016-09-30 08:57:30]

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

[kwiberg-webrtc, 2016-09-30 08:57:31]

This fixes a case where the code basically did

  #if DCHECK_ALWAYS_ON

which isn't allowed now that we use -Wundef, since we don't make sure to always
define DCHECK_ALWAYS_ON. Along with a bunch of other places it was changed to
use RTC_DCHECK_IS_ON instead, which *is* always defined.

[ossu, 2016-09-30 09:08:49]

https://codereview.webrtc.org/2384693002/diff/1/webrtc/base/criticalsection.h
File webrtc/base/criticalsection.h (right):

https://codereview.webrtc.org/2384693002/diff/1/webrtc/base/criticalsection.h...
webrtc/base/criticalsection.h:41: #define CS_DEBUG_CHECKS (!!RTC_DCHECK_IS_ON)
Why!!?
uhm
I mean, why "!!"? :)
Isn't RTC_DCHECK_IS_ON 0 or 1, just like CS_DEBUG_CHECKS should be? So just
define CS_DEBUG_CHECKS to RTC_DCHECK_IS_ON or possibly even replace
CS_DEBUG_CHECKS with a check against RTC_DCHECK_IS_ON?

https://codereview.webrtc.org/2384693002/diff/1/webrtc/base/sequenced_task_ch...
File webrtc/base/sequenced_task_checker.h (right):

https://codereview.webrtc.org/2384693002/diff/1/webrtc/base/sequenced_task_ch...
webrtc/base/sequenced_task_checker.h:17: #define ENABLE_SEQUENCED_TASK_CHECKER
(!!RTC_DCHECK_IS_ON)
Alright, so maybe having separate names for the enabled things is a good thing?
Regardless, I don't think !! does anything useful here.

[kwiberg-webrtc, 2016-09-30 09:14:21]

https://codereview.webrtc.org/2384693002/diff/1/webrtc/base/criticalsection.h
File webrtc/base/criticalsection.h (right):

https://codereview.webrtc.org/2384693002/diff/1/webrtc/base/criticalsection.h...
webrtc/base/criticalsection.h:41: #define CS_DEBUG_CHECKS (!!RTC_DCHECK_IS_ON)
On 2016/09/30 09:08:49, ossu wrote:
> Why!!?
> uhm
> I mean, why "!!"? :)
> Isn't RTC_DCHECK_IS_ON 0 or 1, just like CS_DEBUG_CHECKS should be? So just
> define CS_DEBUG_CHECKS to RTC_DCHECK_IS_ON or possibly even replace
> CS_DEBUG_CHECKS with a check against RTC_DCHECK_IS_ON?

Will do (see other comment).

https://codereview.webrtc.org/2384693002/diff/1/webrtc/base/sequenced_task_ch...
File webrtc/base/sequenced_task_checker.h (right):

https://codereview.webrtc.org/2384693002/diff/1/webrtc/base/sequenced_task_ch...
webrtc/base/sequenced_task_checker.h:17: #define ENABLE_SEQUENCED_TASK_CHECKER
(!!RTC_DCHECK_IS_ON)
On 2016/09/30 09:08:49, ossu wrote:
> Alright, so maybe having separate names for the enabled things is a good
thing?
> Regardless, I don't think !! does anything useful here.

It causes ENABLE_SEQUENCED_TASK_CHECKER to be set to either 0 or 1 regardless of
what true and false values RTC_DCHECK_IS_ON uses. But fine, I guess it isn't
100% necessary. Will remove.

[kjellander_webrtc, 2016-09-30 10:42:28]

I think I'll leave this review to the ones that knows the language better (ossu
that is)

https://codereview.webrtc.org/2384693002/diff/1/webrtc/base/criticalsection.h
File webrtc/base/criticalsection.h (right):

https://codereview.webrtc.org/2384693002/diff/1/webrtc/base/criticalsection.h...
webrtc/base/criticalsection.h:41: #define CS_DEBUG_CHECKS (!!RTC_DCHECK_IS_ON)
Ah, I was going to ask what (!!RTC_DCHECK_IS_ON) means, thanks for clarifying.

[kwiberg-webrtc, 2016-09-30 11:20:40]

kwiberg@webrtc.org changed reviewers:
- kjellander@webrtc.org

[kwiberg-webrtc, 2016-09-30 11:20:41]

https://codereview.webrtc.org/2384693002/diff/1/webrtc/base/criticalsection.h
File webrtc/base/criticalsection.h (right):

https://codereview.webrtc.org/2384693002/diff/1/webrtc/base/criticalsection.h...
webrtc/base/criticalsection.h:41: #define CS_DEBUG_CHECKS (!!RTC_DCHECK_IS_ON)
On 2016/09/30 10:42:28, kjellander_webrtc wrote:
> Ah, I was going to ask what (!!RTC_DCHECK_IS_ON) means, thanks for clarifying.

Yes, ! maps 0 to 1 and all other values to 0, so !! maps 0 to 0 and all other
values to 1. It's a standard C idiom; in C++, you can also do pretty much the
same thing by casting to bool.

[kwiberg-webrtc, 2016-10-01 01:05:28]

New patch set uploaded.

[ossu, 2016-10-03 08:33:15]

On 2016/10/01 01:05:28, kwiberg-webrtc wrote:
> New patch set uploaded.

Great name! :)
LGTM

[kwiberg-webrtc, 2016-10-03 08:58:10]

The CQ bit was checked by kwiberg@webrtc.org

[commit-bot: I haz the power, 2016-10-03 08:58:15]

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-03 10:58:44]

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

[commit-bot: I haz the power, 2016-10-03 10:58:45]

Try jobs failed on following builders:
  android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[kwiberg-webrtc, 2016-10-03 12:02:42]

Description was changed from

==========
Test RTC_DCHECK_IS_ON instead of checking DCHECK_ALWAYS_ON everywhere

The former is always defined (by webrtc/base/checks.h) to either 0 or
1, whereas the latter isn't necessarily defined.

BUG=webrtc:6451
==========

to

==========
Test RTC_DCHECK_IS_ON instead of checking DCHECK_ALWAYS_ON everywhere

The former is always defined (by webrtc/base/checks.h) to either 0 or
1, whereas the latter isn't necessarily defined.

NOTRY=true
BUG=webrtc:6451
==========

[kwiberg-webrtc, 2016-10-03 12:02:56]

The CQ bit was checked by kwiberg@webrtc.org

[commit-bot: I haz the power, 2016-10-03 12:03:05]

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-03 12:04:24]

Description was changed from

==========
Test RTC_DCHECK_IS_ON instead of checking DCHECK_ALWAYS_ON everywhere

The former is always defined (by webrtc/base/checks.h) to either 0 or
1, whereas the latter isn't necessarily defined.

NOTRY=true
BUG=webrtc:6451
==========

to

==========
Test RTC_DCHECK_IS_ON instead of checking DCHECK_ALWAYS_ON everywhere

The former is always defined (by webrtc/base/checks.h) to either 0 or
1, whereas the latter isn't necessarily defined.

NOTRY=true
BUG=webrtc:6451
==========

[commit-bot: I haz the power, 2016-10-03 12:04:25]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-10-03 12:04:28]

Description was changed from

==========
Test RTC_DCHECK_IS_ON instead of checking DCHECK_ALWAYS_ON everywhere

The former is always defined (by webrtc/base/checks.h) to either 0 or
1, whereas the latter isn't necessarily defined.

NOTRY=true
BUG=webrtc:6451
==========

to

==========
Test RTC_DCHECK_IS_ON instead of checking DCHECK_ALWAYS_ON everywhere

The former is always defined (by webrtc/base/checks.h) to either 0 or
1, whereas the latter isn't necessarily defined.

NOTRY=true
BUG=webrtc:6451

Committed: https://crrev.com/ab0b929321d37669165d5795268fa10a8c97ec5b
Cr-Commit-Position: refs/heads/master@{#14474}
==========

[commit-bot: I haz the power, 2016-10-03 12:04:30]

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

[guidou, 2016-10-03 15:32:21]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.webrtc.org/2384083004/ by guidou@webrtc.org.

The reason for reverting is: This CL breaks FYI bots with a compile error.

Sample error:
jingle/glue/thread_wrapper.cc -o obj/jingle/jingle_glue/thread_wrapper.o
In file included from ../../jingle/glue/thread_wrapper.cc:5:
In file included from ../../jingle/glue/thread_wrapper.h:16:
In file included from ../../base/message_loop/message_loop.h:17:
In file included from ../../base/memory/ref_counted.h:19:
../../base/logging.h:598:1: error: call to 'MakeCheckOpString' is ambiguous
DEFINE_CHECK_OP_IMPL(EQ, ==)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../base/logging.h:592:17: note: expanded from macro 'DEFINE_CHECK_OP_IMPL'
    else return MakeCheckOpString(v1, v2, names); \
                ^~~~~~~~~~~~~~~~~
../../jingle/glue/thread_wrapper.cc:46:3: note: in instantiation of function
template specialization 'logging::CheckEQImpl<rtc::Thread *,
jingle_glue::JingleThreadWrapper *>' requested here
  DCHECK_EQ(rtc::Thread::Current(), current());
  ^
../../base/logging.h:748:31: note: expanded from macro 'DCHECK_EQ'
#define DCHECK_EQ(val1, val2) DCHECK_OP(EQ, ==, val1, val2)
                              ^
../../base/logging.h:721:18: note: expanded from macro 'DCHECK_OP'
      ::logging::Check##name##Impl((val1), (val2),                     \
                 ^
<scratch space>:102:1: note: expanded from here
CheckEQImpl
^
../../base/logging.h:555:14: note: candidate function [with t1 = rtc::Thread *,
t2 = jingle_glue::JingleThreadWrapper *]
std::string* MakeCheckOpString(const t1& v1, const t2& v2, const char* names) {
             ^
../../third_party/webrtc/base/checks.h:122:14: note: candidate function [with t1
= rtc::Thread *, t2 = jingle_glue::JingleThreadWrapper *]
std::string* MakeCheckOpString(const t1& v1, const t2& v2, const char* names) {
             ^
In file included from ../../jingle/glue/thread_wrapper.cc:5:
In file included from ../../jingle/glue/thread_wrapper.h:16:
In file included from ../../base/message_loop/message_loop.h:17:
In file included from ../../base/memory/ref_counted.h:19:
../../base/logging.h:598:1: error: call to 'MakeCheckOpString' is ambiguous
DEFINE_CHECK_OP_IMPL(EQ, ==)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../base/logging.h:592:17: note: expanded from macro 'DEFINE_CHECK_OP_IMPL'
    else return MakeCheckOpString(v1, v2, names); \
                ^~~~~~~~~~~~~~~~~
../../jingle/glue/thread_wrapper.cc:81:3: note: in instantiation of function
template specialization 'logging::CheckEQImpl<jingle_glue::JingleThreadWrapper
*, jingle_glue::JingleThreadWrapper *>' requested here
  DCHECK_EQ(this, JingleThreadWrapper::current());
  ^
../../base/logging.h:748:31: note: expanded from macro 'DCHECK_EQ'
#define DCHECK_EQ(val1, val2) DCHECK_OP(EQ, ==, val1, val2)
                              ^
../../base/logging.h:721:18: note: expanded from macro 'DCHECK_OP'
      ::logging::Check##name##Impl((val1), (val2),                     \
                 ^
<scratch space>:5:1: note: expanded from here
CheckEQImpl
^
../../base/logging.h:555:14: note: candidate function [with t1 =
jingle_glue::JingleThreadWrapper *, t2 = jingle_glue::JingleThreadWrapper *]
std::string* MakeCheckOpString(const t1& v1, const t2& v2, const char* names) {
             ^
../../third_party/webrtc/base/checks.h:122:14: note: candidate function [with t1
= jingle_glue::JingleThreadWrapper *, t2 = jingle_glue::JingleThreadWrapper *]
std::string* MakeCheckOpString(const t1& v1, const t2& v2, const char* names) {
             ^
In file included from ../../jingle/glue/thread_wrapper.cc:5:
In file included from ../../jingle/glue/thread_wrapper.h:16:
In file included from ../../base/message_loop/message_loop.h:17:
In file included from ../../base/memory/ref_counted.h:19:
../../base/logging.h:598:1: error: call to 'MakeCheckOpString' is ambiguous
DEFINE_CHECK_OP_IMPL(EQ, ==)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../base/logging.h:592:17: note: expanded from macro 'DEFINE_CHECK_OP_IMPL'
    else return MakeCheckOpString(v1, v2, names); \
                ^~~~~~~~~~~~~~~~~
../../jingle/glue/thread_wrapper.cc:82:3: note: in instantiation of function
template specialization 'logging::CheckEQImpl<jingle_glue::JingleThreadWrapper
*, rtc::Thread *>' requested here
  DCHECK_EQ(this, rtc::Thread::Current());
  ^
../../base/logging.h:748:31: note: expanded from macro 'DCHECK_EQ'
#define DCHECK_EQ(val1, val2) DCHECK_OP(EQ, ==, val1, val2)
                              ^
../../base/logging.h:721:18: note: expanded from macro 'DCHECK_OP'
      ::logging::Check##name##Impl((val1), (val2),                     \
                 ^
<scratch space>:12:1: note: expanded from here
CheckEQImpl
^
../../base/logging.h:555:14: note: candidate function [with t1 =
jingle_glue::JingleThreadWrapper *, t2 = rtc::Thread *]
std::string* MakeCheckOpString(const t1& v1, const t2& v2, const char* names) {
             ^
../../third_party/webrtc/base/checks.h:122:14: note: candidate function [with t1
= jingle_glue::JingleThreadWrapper *, t2 = rtc::Thread *]
std::string* MakeCheckOpString(const t1& v1, const t2& v2, const char* names) {
             ^
3 errors generated.

.