NDEBUG option is needed for any inInlineBoxDetach usage.
Created attachment 203586 [details] Patch
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?
(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). :)
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.
Created attachment 203647 [details] Patch
(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.
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.
Created attachment 203656 [details] Patch
Thanks for the review and took ap's comment into consideration. :)
Comment on attachment 203656 [details] Patch Clearing flags on attachment: 203656 Committed r151164: <http://trac.webkit.org/changeset/151164>
All reviewed patches have been landed. Closing bug.