Comments (13)
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 ?
from albany.
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?
from albany.
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.
from albany.
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 );
from albany.
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.
from albany.
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.
from albany.
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.
from albany.
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.
from albany.
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.
from albany.
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.
from albany.
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.
from albany.
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.
from albany.
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.
from albany.
Related Issues (20)
- Tpetra CudaUVM flags for when Kokkos_ENABLE_CUDA_UVM goes away
- Undefined references when using new Cuda-UVM flags HOT 9
- Nightly test failures HOT 39
- corePDEs_SideSetLaplacian_3D failing on weaver after epetra removal HOT 6
- Clean up nightlies to turn off Epetra HOT 4
- PyAlbany MatrixOperations test failing in spack nightly HOT 18
- Attaway sems modules are broken, causing those tests to fail
- Failing PyAlbany test in spack nightly build HOT 4
- Failing nightly builds due to Omegah HOT 2
- Rework nightlies on CEE for new RHEL8 OS HOT 1
- Can not finish albany spack install since trilinos failed to be installed HOT 6
- Failing attaway nightlies due to Netcdf/Pnetcdf HOT 28
- Weaver failing nightlies HOT 10
- Failing landIce_FO_GIS_AdjointSensitivity_Thickness test in nightly spack build HOT 12
- Failing corePDEs_SteadyHeatConstrainedOpt2D_Conductivity_Dist_Param_Restart test in some nightlies HOT 3
- Failing corePDEs_SideSetLaplacian_3D in CUDA weaver build HOT 2
- Failing landIce_FO_MMS_FO tests on some platforms HOT 8
- Failing corePDEs_SideSetLaplacian_3D test in weaver build HOT 2
- Get CEE clang builds back up on cee-compute001 and cee-compute005
- CALI build failure in nightlies HOT 3
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from albany.