<div dir="ltr">Hi,<br><div class="gmail_extra"><br></div><div class="gmail_extra">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.</div>
<div class="gmail_extra"><br></div><div class="gmail_extra">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.</div>
<div class="gmail_extra"><br></div><div class="gmail_extra">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.</div>
<div class="gmail_extra"><br></div><div class="gmail_extra">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.</div>
<div class="gmail_extra"><br></div><div class="gmail_extra">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.</div>
<div class="gmail_extra"><br></div><div class="gmail_extra">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.</div>
<div class="gmail_extra"><br></div><div class="gmail_extra">Just some thoughts, may not have answered/commented on all the issues raised.</div><div class="gmail_extra"><br></div><div class="gmail_extra">Best regards,</div>
<div class="gmail_extra">Teemu</div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Dec 5, 2013 at 10:44 AM, Szilárd Páll <span dir="ltr"><<a href="mailto:pall.szilard@gmail.com" target="_blank">pall.szilard@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">On Thu, Dec 5, 2013 at 1:31 AM, Mark Abraham (Code Review)<br>
<<a href="mailto:gerrit@gerrit.gromacs.org">gerrit@gerrit.gromacs.org</a>> wrote:<br>> 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).<br>
><br>
> 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.<br>
><br>
> 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.<br>
><br>
> 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.<br>
<br>
I also find it somewhat confusing the above arguments about assert and<br>
i) I'm not sure whether you are suggesting that asserts should be<br>
another kind of gmx_incons and should only be used in setup code ii)<br>
I have the feeling that you may be missing a subtle, but important<br>
point about the use of asserts (more below).<br>
<br>
Here are a few personal opinions. Assert should IMO never end up in<br>
release code and therefore asserts could and should be used liberally<br>
in the code. When you we want meaningful (at least to a developer)<br>
output to be displayed together with context info, we should be using<br>
gmx_incons() instead!<br>
<br>
Secondly, asserts do not only have the role of checking things at<br>
run-time, but an important role is to express constraints on the<br>
inputs of the function in an easy to read manner. Personally, I'm not<br>
sure it matters very much to have context information in assertion<br>
errors (gmx_incons is for that). At an assertion failure one would<br>
anyway start by attaching a debugger and as we should only allow<br>
asserts to compile into RelWithDebSymbols and Debug builds, one won't<br>
even need to recompile.<br></blockquote><div> </div></div></div></div>