Bug 209677 – Move from NDEBUG to ASSERT_ENABLED in all places

• Component: New Bugs
• Status: NEW

[Keith Miller]

Move from NDEBUG to ASSERT_ENABLED in all places

[Keith Miller]

Created attachment 394747 [details]
Patch

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Darin Adler]

This is a good change.

There’s a bit of irony in all this motion, though.

What NDEBUG *really* means is "disable the assertions in <assert.h> in the standard library"; we just decided to key off it for some of our own assertions too.

There’s no reason we *need* to set NDEBUG when we turn on optimizations, so we could also have accomplished some of what we wanted to here by simply *not* setting NDEBUG in the Release+Assert build.

Worth finding out if we are using NDEBUG for anything any more after this. And if so, is it something that should really be tied to <assert.h> or not. We could use a macro with a name that makes more sense to us instead.

[Alexey Proskuryakov]

This looks like a mass replace. I'm not certain about changes like this:

-#ifndef NDEBUG
+#if ASSERT_ENABLED
         const char* debugName() const
         {
             return BankInfo::debugName(regID());

Why would this debugging facility be tied to ASSERT_ENABLED?

[Darin Adler]

If we’re truly doing a global replace, then we should just "not set NDEBUG" instead.

Disentangling other debugging mechanisms from ASSERT_ENABLED might be useful, but can’t be done with global replace.

[Keith Miller]

(In reply to Alexey Proskuryakov from comment #4)
> This looks like a mass replace. I'm not certain about changes like this:
> 
> -#ifndef NDEBUG
> +#if ASSERT_ENABLED
>          const char* debugName() const
>          {
>              return BankInfo::debugName(regID());
> 
> Why would this debugging facility be tied to ASSERT_ENABLED?

I'm not really sure why that's only enabled on debug anyway 🤷‍♂️...

[Keith Miller]

(In reply to Darin Adler from comment #5)
> If we’re truly doing a global replace, then we should just "not set NDEBUG"
> instead.

Maybe, on the other hand writing code that says #ifndef NDEBUG is very confusing. 95%+ of the time you're adding code for assertions. IMO, it makes more sense to have an ASSERT_ENABLED over the double negation everywhere.

> 
> Disentangling other debugging mechanisms from ASSERT_ENABLED might be
> useful, but can’t be done with global replace.

I'm not totally convinced by this since it's not uncommon to have good error messages for your assertions. At least I try to do that. On the other hand I'm not sure why we are not always building it anyway. Maybe because of binary size?