| Summary: | Move from NDEBUG to ASSERT_ENABLED in all places | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Keith Miller <keith_miller> | ||||
| Component: | New Bugs | Assignee: | Keith Miller <keith_miller> | ||||
| Status: | NEW --- | ||||||
| Severity: | Normal | CC: | aboxhall, alecflett, apinheiro, ap, beidson, benjamin, berto, calvaris, cdumez, cfleizach, cgarcia, cmarcelo, darin, dino, dmazzoni, eric.carlson, esprehn+autocc, ews-watchlist, fmalita, fred.wang, galpeter, glenn, graouts, gustavo, gyuyoung.kim, hi, hta, jamesr, japhet, jcraig, jdiggs, jer.noble, jiewen_tan, joepeck, jsbell, kangil.han, kondapallykalyan, luiz, macpherson, mark.lam, menard, mifenton, mmaxfield, msaboff, noam, pdr, philipj, pnormand, ryuan.choi, saam, sabouhallawa, samuel_white, schenney, sergio, simon.fraser, tommyw, tonikitoo, toyoshim, tzagallo, vjaquez, yutak, zeno | ||||
| Priority: | P2 | ||||||
| Version: | WebKit Nightly Build | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| Attachments: |
|
||||||
|
Description
Keith Miller
2020-03-27 12:38:57 PDT
Created attachment 394747 [details]
Patch
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 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. 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?
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. (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 🤷♂️... (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? |