Updated styleguide re. CHECK()s and friends

[Peter Boström]

Hi folks!

Over at cxx@ we've been making a recent change to our C++ styleguide to prefer using CHECKs over DCHECKs by default with an exception if the invariant is too expensive to verify in production. You can see the specific changes here, our C++ styleguide is here and an expanded doc on CHECKs (and friends) is here.

Another change is that DCHECK and NOTREACHED failures should be seen as P1 bugs. Even if they are not validated in production builds, the side effects of blowing past one of these in a production build often has severe implications that manifest as security bugs.

We've also introduced NOTREACHED_NORETURN(), a [[noreturn]] version of NOTREACHED(). M112 will report NOTREACHED() violations in production builds, please pay extra attention to bugs filed here to avoid instability spikes as we migrate to the [[noreturn]] version. Once migration is done this'll be renamed back to NOTREACHED() and there won't be a non-fatal version.

Thanks,

Peter

[Peter Boström]

I'm out on parental leave until ~early Nov, if you could file a bug for these specifically and assign them to me that'd be helpful so I don't lose track of this. For sequence checking I've been doing some speedup work as part of crbug.com/1372121 which is a catch-all bug for DCHECK-build performance. There's still probably significant improvements that can be done until sequence checking stops showing up in DCHECK-build profiling in the wild, then we could reasonably apply them elsewhere.

Either file one bug per desired macro to have a CHECK_ equivalent or one for path/to/your/area/of/ownership can't stop using DCHECK due to these macros missing a CHECK equivalent, which will either be resolved with new guidance to avoid these specifically or new macros that you'd reasonably be able to use.

On Mon, Jun 26, 2023 at 2:26 AM Domenic Denicola <dom...@chromium.org> wrote:

Are there any plans to update various DCHECK-based helpers? For example DCHECK_CALLED_ON_VALID_SEQUENCE or DCHECK_STATE_TRANSITION. Our team has been converting our DCHECKs to CHECKs, but we don't have an easy solution for those. Maybe they are considered too expensive to convert?

Adding guidance on them to the CHECK-and-friends doc would be much appreciated.

[Domenic Denicola]

Are there any plans to update various DCHECK-based helpers? For example DCHECK_CALLED_ON_VALID_SEQUENCE or DCHECK_STATE_TRANSITION. Our team has been converting our DCHECKs to CHECKs, but we don't have an easy solution for those. Maybe they are considered too expensive to convert?

Adding guidance on them to the CHECK-and-friends doc would be much appreciated.

On Saturday, March 18, 2023 at 6:10:25 AM UTC+9 Peter Boström wrote:

[Domenic Denicola]

Done! I filed bugs for DCHECK_CURRENTLY_ON, DCHECK_CALLED_ON_VALID_SEQUENCE, and DCHECK_STATE_TRANSITION. All marked as blocking our team bug.

Thanks for all your work here!