Re: [PATCH 1/3] Documentation/CodingGuidelines: explain why assert is bad

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Nov 22, 2017, 22:59 UTC
Message-ID: <20171122225913.GF11671@aiede.mtv.corp.google.com>
In-Reply-To: <20171122223827.26773-2-sbeller@google.com>

Hi,

Stefan Beller wrote:

Show 8 quoted lines

> --- a/Documentation/CodingGuidelines
> +++ b/Documentation/CodingGuidelines
> @@ -386,6 +386,9 @@ For C programs:
>   - Use Git's gettext wrappers to make the user interface
>     translatable. See "Marking strings for translation" in po/README.
>  
> + - Prefer the BUG() macro over asserts, as asserts requires that the
> +   NDEBUG flag is unset on compilation to work.

nit: is there some logical place in the list of C guidelines this should go at, instead of the last item? Maybe near the top, since this is one of those straightforward cases and we're just saying that this is the startegy for asserting invariants that this project prefers.

Separately: I am not sure we currently universally prefer BUG_ON()
over assert().  In theory, assert() is fine wherever you don't care
whether the assertion is checked at run-time --- in other words, it is
a fancy form of comment.  BUG_ON() is useful for defensive checks that
you *expect* to never trip but want to rely on afterwards.

In a certain ideal world, the preference would be reversed: you'd want to use assert() wherever you can and require the compiler to check that all assert()s are verifiable at compile time. A check that a static analyzer can verify is more valuable than a run-time check. When a compile-time check is not possible, you'd have to fall back to BUG_ON().

But we don't live in that ideal world. I'm not aware of any widely available tools for checking assert()s. So maybe it makes sense to forbid assert() in our codebase entirely.

That could be enforced using something like check-non-portable-shell.pl, except

- run on C files instead of sh files
- run on Git's source code instead of t/

What do you think?

Thanks, Jonathan