Bug 117146 – Use ASSERT_DISABLED option for assertion purpose code in InlineBox

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Kangil Han]

NDEBUG option is needed for any inInlineBoxDetach usage.

[Kangil Han]

Created attachment 203586 [details]
Patch

[Tim Horton]

Comment on attachment 203586 [details]
Patch

This is already building and has been a long time, and ASSERTs are compiled out in release builds, so why do we need this?

[Kangil Han]

(In reply to comment #2)
> (From update of attachment 203586 [details])
> This is already building and has been a long time, and ASSERTs are compiled out in release builds, so why do we need this?

Yes, this patch doesn't change any behavior in release(even debug) build. But would bring us a code consistency(readability). :)

[Alexey Proskuryakov]

Comment on attachment 203586 [details]
Patch

You are right that the code is not quite correct - NDEBUG and ASSERT_DISABLED usually go hand in hand, but sometimes people do use release builds with assertions enabled, and this code will fail to build in this configuration.

A better way to fix this is to change NDEBUG #ifs to ASSERT_DISABLED ones elsewhere in this file.

[Kangil Han]

Created attachment 203647 [details]
Patch

[Kangil Han]

(In reply to comment #4)
> (From update of attachment 203586 [details])
> You are right that the code is not quite correct - NDEBUG and ASSERT_DISABLED usually go hand in hand, but sometimes people do use release builds with assertions enabled, and this code will fail to build in this configuration.
> 
> A better way to fix this is to change NDEBUG #ifs to ASSERT_DISABLED ones elsewhere in this file.

Took ap's comment into consideration.

[Alexey Proskuryakov]

Comment on attachment 203647 [details]
Patch

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

> Source/WebCore/rendering/InlineBox.cpp:35
> -#ifndef NDEBUG
> +#if !ASSERT_DISABLED
>  #include <stdio.h>
>  #endif

stdio.h is needed for code below that is genuinely DEBUG-specific, so this change is not correct.

> Source/WebCore/rendering/InlineBox.cpp:95
> +#if !ASSERT_DISABLED
>      ASSERT(inInlineBoxDetach);
> -
> +#endif

This instance doesn't need to be guarded - ASSERT macro itself has the check.

> Source/WebCore/rendering/InlineBox.cpp:101
> -#ifndef NDEBUG
> +#if !ASSERT_DISABLED
>  const char* InlineBox::boxName() const

These functions are DEBUG specific, they are not related to assertions at all. So, NDEBUG was correct here.

> Source/WebCore/rendering/InlineBox.cpp:395
> -#ifndef NDEBUG
> +#if !ASSERT_DISABLED

Ditto.

> Source/WebCore/rendering/InlineBox.h:111
> -#ifndef NDEBUG
> +#if !ASSERT_DISABLED

Ditto.

> Source/WebCore/rendering/InlineBox.h:439
> -#ifndef NDEBUG
> +#if !ASSERT_DISABLED

Ditto.

[Kangil Han]

Created attachment 203656 [details]
Patch

[Kangil Han]

Thanks for the review and took ap's comment into consideration. :)

[WebKit Commit Bot]

Comment on attachment 203656 [details]
Patch

Clearing flags on attachment: 203656

Committed r151164: <http://trac.webkit.org/changeset/151164>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.