[gmx-developers] use of assert

[Teemu Murtola]

Hi,

here are some comments on why the C++ implementation is like it is. I agree
with the general approach of using asserts liberally, everywhere in the
code, and asserts used for this purpose in any performance-sensitive code
should get turned off by NDEBUG.

But there is a lot of code where performance is not an issue, and it is a
lot better to fail with a clear error message than, e.g., segfault (or even
worse, producing incorrect results because some invariant gets violated).
Even if the user would just copy-paste the error they get into their
complaint on gmx-users, it is a lot easier to start looking at the issue
than if there is a segfault or some mysterious failure.

Another factor to consider is that we are providing an API for users to
call. And I think that for less tech-savvy users, they will likely just use
a release-built library for that. Consistency checks in those builds for
correct API calls etc. can serve the end user to spot their own mistakes.
Similarly, since a lot of testing people do on Gromacs is
performance-sensitive, release builds are quite common also during
development. And the asserts can spot issues in such testing only if they
are not disabled.

It is a matter of semantics whether you call such release-enabled checks
GMX_RELEASE_ASSERT(), or gmx_incons(), or whatever. The purpose of the
check is exactly the same, and I would argue based on the above that those
should generally go into release builds as well. We can change the name
from an assert to something else if people would find that less confusing.

On readability, I agree that expressing constraints through asserts is
often good. But unless the constraint is something trivial (like requiring
non-negative input), the condition itself doesn't often document _why_ this
is necessary, or what is the actual error that triggers the assert. Because
of this, the GMX_ASSERT macro takes a separate string to describe the
error. While this requires some effort to write, I think forcing people to
think about such documentation makes it easier for others to understand the
code.

Another purpose for which a custom assert implementation makes sense is for
unit testing. Right now, if an assert (or gmx_fatal or similar) is
triggered during a test, it will simply terminate the test program, and
there is no per-test output visible in Jenkins. It will simply say that the
build is unstable, and only the console output indicates what the error is
(if you find the assertion message from there). A custom implementation
that provides a hook to customize the assertion behavior would allow
providing more reasonable output, extracting the message into the XML file
that Jenkins parses.

Just some thoughts, may not have answered/commented on all the issues
raised.

Best regards,
Teemu

On Thu, Dec 5, 2013 at 10:44 AM, Szilárd Páll <pall.szilard at gmail.com>wrote:

> On Thu, Dec 5, 2013 at 1:31 AM, Mark Abraham (Code Review)
> <gerrit at gerrit.gromacs.org> wrote:
> > 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.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://maillist.sys.kth.se/pipermail/gromacs.org_gmx-developers/attachments/20131206/58ff3284/attachment.html>