[webkit-reviews] review granted: [Bug 205776] Convert ASSERT_DISABLED to ASSERT_ENABLED, and fix some tests of NDEBUG that should actually test for ASSERT_ENABLED. : [Attachment 386819] proposed patch.

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 205776: Convert ASSERT_DISABLED to ASSERT_ENABLED, and fix some tests of
NDEBUG that should actually test for ASSERT_ENABLED.
https://bugs.webkit.org/show_bug.cgi?id=205776

Attachment 386819: proposed patch.

https://bugs.webkit.org/attachment.cgi?id=386819&action=review




--- Comment #15 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 386819
  --> https://bugs.webkit.org/attachment.cgi?id=386819
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=386819&action=review

r=me

I suggest also:
1. Filing a bug to get rid of more "ifndef NDEBUG"
2. Get all layout tests to pass when enabling ASSERT_ENABLED on release builds
(presumably [1] will help with this).
3. Send out a PSA to webkit-dev so folks stop using "ifndef NDEBUG" for most
assertions. (Some still might make sense if they're super costly in terms of
perf.)

> Source/JavaScriptCore/ChangeLog:8
> +	   This patch did the following changes:

nit: probably should go in WTF/ChangeLog?

> Source/JavaScriptCore/ChangeLog:17
> +	      as well.	Well do that in another patch.	For now, they are left
as is to

"Well" => "We'll"

> Source/WTF/wtf/OptionSet.h:82
> +#ifndef NDEBUG

why isn't this ASSERT_ENABLED?

> Source/WTF/wtf/Platform.h:1050
> +   do debug assertion checks unconditionally (e.g.  treat a debug ASSERT

nit: "	" => " "

> Source/WebCore/html/HTMLTableRowsCollection.cpp:49
> +#else // not ASSERT_ENABLED

nit: why "not" in front? Do we do that in places?

> Source/WebCore/html/HTMLTextFormControlElement.cpp:654
> +    // FIXME: This assertion code was never built, has bit rotted, and needs
to be fixed before it can be enabled:
> +    // https://bugs.webkit.org/show_bug.cgi?id=205706.
> +#if ASSERT_ENABLED
>      VisiblePosition visiblePosition = passedPosition;
>      unsigned indexComputedByVisiblePosition = 0;
>      if (visiblePosition.isNotNull())
>	   indexComputedByVisiblePosition =
WebCore::indexForVisiblePosition(innerText, visiblePosition, false /*
forSelectionPreservation */);
>      ASSERT(index == indexComputedByVisiblePosition);

should we just delete it?