[gmx-developers] use of assert

[Szilárd Páll]

Moving a discussion on the use of asserts from the gerrit review of a
change to to the mailing list.

On Thu, Dec 5, 2013 at 1:31 AM, Mark Abraham (Code Review)
<gerrit at gerrit.gromacs.org> wrote:
> Mark Abraham has posted comments on this change.
>
> Change subject: Free-energy now works with the Verlet scheme
[...]
> @asserts:
>
> 4.6 used a mixture of the standard assert.h (which respected NDEBUG if the implementation did) and our include/assert.h (which did not, but gave context information).
>
> There is still code using the former. C++ code (i.e. mostly written by Teemu) uses gromacs/utility/gmxassert.h which is supposed to provide the best of all the worlds, and leave open the option to implement it with an exception. The latter provides versions that will or won't respect NDEBUG, if the need exists. Making use of it is awkward thought, because it adds the NDEBUG dimension to Jenkins, which is problematic.
>
> There is no assert code left that does not respect NDEBUG (unless explicitly requested). However, I think being concerned about the performance impact is a mis-placed priority. A user should never see an assertion, because all they can do is shrug and report a bug with no context, so there is little point having them in production code. Failing the assert would normally lead to some ugly failure in the next few lines, and the assert is only slightly better than that - the real problem always happened somewhere else! Better modularity and encapsulation will help limit the range of "somewhere else," but that is a long-term project.
>
> With the above in mind, I think we should be making copious use of asserts, particularly in setup code. The use in nbnxn_search.c is not setup code, but the use of NDEBUG makes the point moot.

I also find it somewhat confusing the above arguments about assert and
i) I'm not sure whether you are suggesting that asserts should be
another kind of gmx_incons and should only be used in setup code  ii)
I have the feeling that you may be missing a subtle, but important
point about the use of asserts (more below).

Here are a few personal opinions. Assert should IMO never end up in
release code and therefore asserts could and should be used liberally
in the code. When you we want meaningful (at least to a developer)
output to be displayed together with context info, we should be using
gmx_incons() instead!

Secondly, asserts do not only have the role of checking things at
run-time, but an important role is to express constraints on the
inputs of the function in an easy to read manner. Personally, I'm not
sure it matters very much to have context information in assertion
errors (gmx_incons is for that). At an assertion failure one would
anyway start by attaching a debugger and as we should only allow
asserts to compile into RelWithDebSymbols and Debug builds, one won't
even need to recompile.

You do make a good point about the need to have much more build
configs in jenkins that use RelWithDebInfo configs - perhaps we should
even duplicate all configs and run them both in Release and
RelWithDebInfo (perhaps the latter only in nightly mode?).

Cheers,
--
Szilárd