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

[Jonathan Nieder <jrnieder@gmail.com>]

Hi,

Stefan Beller wrote:

> --- 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