Move away from TEUCHOS_TEST_FOR_EXCEPTION towards assert

[jtostie]

No description provided.

[ibaned]

I'm actually surprised someone besides me prefers assert(). Its less work to type out and signals are somewhat easier to catch and trace than exceptions. However, it doesn't print information quite as flexibly. Any rationale I missed here ?

[gahansen]

I also prefer assert(). We might consider replacing the TEUCHOS_TEST_FOR_EXCEPTION calls with a macro | function defined in a toplevel Albany include file that has the option of printing a more extensive message and then finally calls assert(). Does anyone see issues with moving toward assert() or have any counter-arguments?

[jtostie]

If I recall correctly, @lxmota mentioned that the TEUCHOS macro is a nightmare in the context of debugging. Best to let him comment though. I have no issue moving towards assert.

[rppawlo]

There is one major difference between teuchos and assert. Teuchos will always be checked in all build types. You can't disable it. In release builds (that define NDEBUG), the code within the assert will never be executed. So when changing over code, make sure that the asserts aren't making calls to functions that must always be executed.

Also, you can easily debug the teuchos macro. There is a dumy function that you can set a breakpoint on:

/** \brief The only purpose for this function is to set a breakpoint.
    \ingroup TestForException_grp */
TEUCHOSCORE_LIB_DLL_EXPORT void TestForException_break( const std::string &msg );

[ibaned]

I agree that NDEBUG should not make the assert calls disappear. I've got some experience writing custom asserts, so I put up a first draft as PR #35. That PR also switches a few calls from TEUCHOS...EXCEPTION to ALBANY_ASSERT, to get a sense of what it looks like. After we get input from people on the interface and if we think we should switch, I'll merge this in and start replacing calls at will.

[lxmota]

I also prefer assert for the many reasons already mentioned here. But I must say that one of the reasons that I like it is precisely because that macro does nothing if the NDEBUG symbol is defined. That is the standard behavior as defined here and many other places:

http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/assert.h.html

One big issue for me is that if I see standard constructs like assert, I would expect standard behavior. Making it behave in unexpected ways (being active regardless of whether NDEBUG is defined) introduces confusion and in this particular case would degrade performance in a release build. That is way I am so insistent that Albany should be tested by developers in debug mode also, and I do that in my nightly tests.

As to whether there should be an assertion macro like ALBANY_ASSERT that is always active, that is fine with me. It's up the person writing the code whether to use it or not. I personally prefer to treat assertions as truly exceptional and therefore they should only be active in debug mode, and so in my workflow I always work with a debug build.

[agsalin]

Alejandro makes a good point. It is nice to put asserts in freely to test code without concern for performance. On the other hand, it is nice that you can get a good error message when you misspell something in XML when running a performance build. That speaks having two different error checking approach, (1) the standard assert and (2) something that behaves like TEUCHOS_TEST_FOR_EXCEPT but perhaps without having to pick an exception type. Also, I'm thinking that having true be a good result, like assert, would make it more uniform.

[ibaned]

The other feature to keep in mind as @gahansen pointed out is the ability to print more information. I think we could do something like this:

                    respects NDEBUG ?
                 yes                 no
          --------------------------------------
extra   y |ALBANY_ASSERT | ALBANY_ALWAYS_ASSERT|
msg ?     --------------------------------------
        n |    assert    |           ???       |
          --------------------------------------

And people choose one of these based on the specific situation.
I'll work on making those two macros do what we just discussed.

[lxmota]

The two-macro solution looks good to me. I would like to request one thing: that both macros take exactly the same arguments, including an error message.

[bartgol]

I also second the two macro solution. Perhaps we can make the ALBANY_ASSERT clearer by calling it ALBANY_DEBUG_ASSERT or something along that line.

If the length of the macro name is not a concern (after all TEUCHOS_TEST_FOR_EXCEPTION is not short either), we could consider adding _VERBOSE to the macro names in the extra msg case:

                    respects NDEBUG ?
                 yes                 no
          ------------------------------------------------------------
extra   y |ALBANY_DEBUG_ASSERT_VERBOSE | ALBANY_ALWAYS_ASSERT_VERBOSE|
msg ?     ------------------------------------------------------------
        n |    ALBANY_DEBUG_ASSERT     |   ALBANY_ALWAYS_ASSERT      |
          ------------------------------------------------------------

Or without the_DEBUG part.

[lxmota]

One issue that I have with TEUCHOS_TEST_FOR_EXCEPTION is that its name is too long.

I propose that we use ALBANY_ASSERT instead of ALBANY_ALWAYS_ASSERT, and ALBANY_EXPECT for ALBANY_DEBUG_ASSERT. The first always fails if the provided boolean (and it would be great if this is enforced, no more checking for ints) value is false, while the second will only fail if NDEBUG is defined.

An optional error message could be the last parameter for both macros, so there would be no need to make the name of the macros longer by adding the postfix _VERBOSE.

[ibaned]

Thanks everyone for your input ! I was able to implement both systems proposed by @lxmota and @bartgol. So there are six new macros available in Albany_Utils.hpp including ALBANY_ASSERT and ALBANY_EXPECT whose message argument is optional.

https://github.com/gahansen/Albany/blob/master/src/Albany_Utils.hpp#L166

I think this should have all the features requested in this thread, so I'll go ahead and close this. Feel free to start replacing TEUCHOS_TEST_FOR_EXCEPTION in places you are particularly interested in, and I may also go through and replace it as time allows. If there is anything wrong with the new macros, please reopen this.

[bartlettroscoe]

The bottom line is that if your code can't provide the "basic guarantee", it needs to abort and not thrown an exception. But if you do abort, it is still nice to produce a nice error message. assert() does not produce a nice error message.