Replace all occurrences of RELEASE_ASSERT in wtf with CHECK.

Replace all occurrences of RELEASE_ASSERT in wtf with CHECK.

BUG=596760
Committed: https://crrev.com/d181f524c11e0d456bc48261334beda61e6f852c
Cr-Commit-Position: refs/heads/master@{#384213}

[kotenkov, 2016-03-29 18:53:12]

kotenkov@yandex-team.ru changed reviewers:
+ haraken@chromium.org

[kotenkov, 2016-03-29 18:53:12]

Are you accepting CLs, converting from RELEASE_ASSERT to CHECK?

I've decided to start small and convert just wtf.
However, there was an issue with RELEASE_ASSERT here:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
It has a NO_RETURN_DUE_TO_CRASH specifier and clang obviously cannot see that
CHECK will crash and gives me "error: function declared 'noreturn' should not
return [-Werror,-Winvalid-noreturn]".
Should i just remove the NO_RETURN_DUE_TO_CRASH or tune CHECK somehow?

Preprocessor output for both variants:
// CHECK output
__attribute((__noreturn__)) void writeTo(LChar*)
{
    !(!(false)) ? (void) 0 : ::logging::LogMessageVoidify() &
(logging::LogMessage("../../third_party/WebKit/Source/wtf/text/StringConcatenate.h",
176, "false").stream());
}


// ASSERT output
__attribute((__noreturn__)) void writeTo(LChar*)
{
    (!(false) ?
(WTFReportAssertionFailure("../../third_party/WebKit/Source/wtf/text/StringConcatenate.h",
176, __PRETTY_FUNCTION__, "false"), (WTFReportBacktrace(), (*(int*)0xfbadbeef =
0), __builtin_trap())) : (void)0);
}

[haraken, 2016-03-30 00:41:14]

LGTM


On 2016/03/29 18:53:12, kotenkov wrote:
> Are you accepting CLs, converting from RELEASE_ASSERT to CHECK?
> 
> I've decided to start small and convert just wtf.
> However, there was an issue with RELEASE_ASSERT here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> It has a NO_RETURN_DUE_TO_CRASH specifier and clang obviously cannot see that
> CHECK will crash and gives me "error: function declared 'noreturn' should not
> return [-Werror,-Winvalid-noreturn]".
> Should i just remove the NO_RETURN_DUE_TO_CRASH or tune CHECK somehow?
> 
> Preprocessor output for both variants:
> // CHECK output
> __attribute((__noreturn__)) void writeTo(LChar*)
> {
>     !(!(false)) ? (void) 0 : ::logging::LogMessageVoidify() &
>
(logging::LogMessage("../../third_party/WebKit/Source/wtf/text/StringConcatenate.h",
> 176, "false").stream());
> }
> 
> 
> // ASSERT output
> __attribute((__noreturn__)) void writeTo(LChar*)
> {
>     (!(false) ?
>
(WTFReportAssertionFailure("../../third_party/WebKit/Source/wtf/text/StringConcatenate.h",
> 176, __PRETTY_FUNCTION__, "false"), (WTFReportBacktrace(), (*(int*)0xfbadbeef
=
> 0), __builtin_trap())) : (void)0);
> }

I think we can just drop the NO_RETURN_DUE_TO_CRASH (since that wouldn't be a
big deal anyway).

[haraken, 2016-03-30 00:41:23]

haraken@chromium.org changed reviewers:
+ yutak@chromium.org

[Yuta Kitamura, 2016-03-30 03:55:17]

https://codereview.chromium.org/1840163002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/wtf/BitArray.h (right):

https://codereview.chromium.org/1840163002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/BitArray.h:46: CHECK(index < arraySize);
I think CHECK_LT is more suitable.

The same applies in all other places where a binary operator is
used within CHECK().

[kotenkov, 2016-03-30 05:55:31]

Description was changed from

==========
Replace most occurrences of RELEASE_ASSERT in wtf with CHECK.

BUG=596760
==========

to

==========
Replace all occurrences of RELEASE_ASSERT in wtf with CHECK.

BUG=596760
==========

[kotenkov, 2016-03-30 05:57:30]

On 2016/03/30 03:55:17, Yuta Kitamura wrote:
>
https://codereview.chromium.org/1840163002/diff/1/third_party/WebKit/Source/w...
> File third_party/WebKit/Source/wtf/BitArray.h (right):
> 
>
https://codereview.chromium.org/1840163002/diff/1/third_party/WebKit/Source/w...
> third_party/WebKit/Source/wtf/BitArray.h:46: CHECK(index < arraySize);
> I think CHECK_LT is more suitable.
> 
> The same applies in all other places where a binary operator is
> used within CHECK().

Done. It's somewhat harder to do programmatically, but it looks better.
PTAL again please.

[kotenkov, 2016-03-30 20:12:15]

There actually are few RELEASE_ASSERTs:
bindings 48
core 80
modules 11
platform 50
web 10
How should I split them into CLs? I can just convert them all here, I think it
would still be readable. Or I can split them by top-level folders, so the proper
owners review them.

As for ASSERTs, these are much more numerous:
bindings 563
core 6994
modules 1596
platform 1781
web 442
wtf 743
I think, I should definitely split them into CLs by top-level folders and maybe
further in at least core.

What do you guys think?

[haraken, 2016-03-30 23:27:08]

On 2016/03/30 20:12:15, kotenkov wrote:
> There actually are few RELEASE_ASSERTs:
> bindings 48
> core 80
> modules 11
> platform 50
> web 10
> How should I split them into CLs? I can just convert them all here, I think it
> would still be readable. Or I can split them by top-level folders, so the
proper
> owners review them.
> 
> As for ASSERTs, these are much more numerous:
> bindings 563
> core 6994
> modules 1596
> platform 1781
> web 442
> wtf 743
> I think, I should definitely split them into CLs by top-level folders and
maybe
> further in at least core.
> 
> What do you guys think?

Splitting by top-level folders sounds good to me.

[Yuta Kitamura, 2016-03-31 04:46:14]

LGTM modulo CHECK_NE and the removal of NO_RETURN_DUE_TO_CRASH

https://codereview.chromium.org/1840163002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/Deque.h (right):

https://codereview.chromium.org/1840163002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/Deque.h:561: CHECK(m_index != m_deque->m_end);
CHECK_NE?

https://codereview.chromium.org/1840163002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/text/StringConcatenate.h (left):

https://codereview.chromium.org/1840163002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/text/StringConcatenate.h:174:
NO_RETURN_DUE_TO_CRASH void writeTo(LChar*)
Why was this removed?

[kotenkov, 2016-03-31 05:50:55]

On 2016/03/31 04:46:14, Yuta Kitamura wrote:
> third_party/WebKit/Source/wtf/Deque.h:561: CHECK(m_index != m_deque->m_end);
> CHECK_NE?

Done.

> third_party/WebKit/Source/wtf/text/StringConcatenate.h:174:
> NO_RETURN_DUE_TO_CRASH void writeTo(LChar*)
> Why was this removed?

See https://codereview.chromium.org/1840163002/#msg2 and
https://codereview.chromium.org/1840163002/#msg3 .

[kotenkov, 2016-03-31 05:51:14]

The CQ bit was checked by kotenkov@yandex-team.ru

[kotenkov, 2016-03-31 05:51:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, yutak@chromium.org
Link to the patchset: https://codereview.chromium.org/1840163002/#ps60001
(title: "Use CHECK_NE instead of CHECK.")

[commit-bot: I haz the power, 2016-03-31 05:51:38]

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

[Yuta Kitamura, 2016-03-31 06:29:14]

On 2016/03/30 00:41:14, haraken wrote:
> I think we can just drop the NO_RETURN_DUE_TO_CRASH (since that wouldn't be a
> big deal anyway).

Hm, I think that would affect compiler optimization.

[kotenkov, 2016-03-31 06:30:18]

The CQ bit was unchecked by kotenkov@yandex-team.ru

[kotenkov, 2016-03-31 06:38:03]

On 2016/03/31 06:29:14, Yuta Kitamura wrote:
> On 2016/03/30 00:41:14, haraken wrote:
> > I think we can just drop the NO_RETURN_DUE_TO_CRASH (since that wouldn't be
a
> > big deal anyway).
> 
> Hm, I think that would affect compiler optimization.

Yes, it will. I guess, the 'no big deal' part was about that this function is
already supposed to never be called and its optimizations doesn't really matter
very much.
Another part is that compiler may be able to tell you that you are calling a
__noreturn__ function and the code after that is dead. I think this is no big
deal too, because you will just see the crash.
Tell me please, if you have other ideas on how to handle the situation, or give
me another explicit lgtm.

[Yuta Kitamura, 2016-03-31 07:04:21]

On 2016/03/31 06:38:03, kotenkov wrote:
> On 2016/03/31 06:29:14, Yuta Kitamura wrote:
> > On 2016/03/30 00:41:14, haraken wrote:
> > > I think we can just drop the NO_RETURN_DUE_TO_CRASH (since that wouldn't
be
> a
> > > big deal anyway).
> > 
> > Hm, I think that would affect compiler optimization.
> 
> Yes, it will. I guess, the 'no big deal' part was about that this function is
> already supposed to never be called and its optimizations doesn't really
matter
> very much.
> Another part is that compiler may be able to tell you that you are calling a
> __noreturn__ function and the code after that is dead. I think this is no big
> deal too, because you will just see the crash.
> Tell me please, if you have other ideas on how to handle the situation, or
give
> me another explicit lgtm.

I'm not opposed to removing noreturn, but it looks a little
bit concerning to me because, theoretically, compilers would
need to emit larger code in all the call sites (they need to
save more registers, etc.).

My guess is that this specific removal wouldn't matter
practically, so I'm supportive of landing it and seeing if
it will break the perf.

[kotenkov, 2016-03-31 07:09:08]

The CQ bit was checked by kotenkov@yandex-team.ru

[commit-bot: I haz the power, 2016-03-31 07:09:29]

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

[commit-bot: I haz the power, 2016-03-31 07:39:58]

Description was changed from

==========
Replace all occurrences of RELEASE_ASSERT in wtf with CHECK.

BUG=596760
==========

to

==========
Replace all occurrences of RELEASE_ASSERT in wtf with CHECK.

BUG=596760
==========

[commit-bot: I haz the power, 2016-03-31 07:39:59]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-03-31 07:41:23]

Description was changed from

==========
Replace all occurrences of RELEASE_ASSERT in wtf with CHECK.

BUG=596760
==========

to

==========
Replace all occurrences of RELEASE_ASSERT in wtf with CHECK.

BUG=596760

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

[commit-bot: I haz the power, 2016-03-31 07:41:24]

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

[tkent, 2016-04-03 17:55:18]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/1854033002/ by tkent@chromium.org.

The reason for reverting is: Caused performance regression; crbug.com/599867.

[Takashi Toyoshima, 2016-04-04 10:40:39]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/1859513002/ by toyoshim@chromium.org.

The reason for reverting is: Caused performance regression..