Define NDEBUG in release mode

[gassmoeller]

Let me start by saying I am not fully aware of the impact of this. This is more of a question: By default cmake would define NDEBUG in anything but debug mode, to disable asserts in the standard library. Deal.II overwrites this default in the DEAL_II_INITIALIZE_CACHED_VARIABLES macro, when it resets CMAKE_CXX_FLAGS and it never defines it itself (it instead uses DEBUG to activate asserts in debug mode). I am not sure if this was made on purpose, but inside ASPECT we have some downstream projects (specifically https://github.com/GeodynamicWorldBuilder/WorldBuilder) that rely on NDEBUG to decide whether to activate asserts or not. Does it make sense to keep cmake's default behavior of defining NDEBUG in release mode? This may even give some performance benefits, but I do not know if it was removed no purpose.

[drwells]

I think external projects should set it themselves (i.e., we should not add it to our list of release flags) - for example, if you do -DCMAKE_BUILD_TYPE=Release, TARGET_LINK_LIBRARIES(myproject PUBLIC ${DEAL_LIBRARIES_RELEASE}) etc. then CMake will define NDEBUG during compilation - this is what I would expect to happen. If you set your own CMAKE_CXX_FLAGS then this behavior is overridden and CMake sets whatever flags you specify.

Does WorldBuilder just set their flags equal to deal.II's flags?

[tamiko]

@gassmoeller To answer your first question: We override CMake's default choices for debug and release builds because our autoconf/make based build system back in the day had a different set of compiler flags and compilation logic.

Regarding NDEBUG - we could definitely set it for our release builds.

I think the major reason why we don't already do that is the simply reason that we don't use assert() internally so it hardly matters for us: Debug asserts for the standard library itself are controlled via -D_GLIBCXX_DEBUG (libstdc++) or -D_LIBCPP_DEBUG (clang's libc++) - and for calling into trilinos/petsc/etc. NDEBUG doesn't play a role either.

[gassmoeller]

Does WorldBuilder just set their flags equal to deal.II's flags?

We compile WorldBuilder with the ASPECT build system and that one just takes the deal.II flags. Yes, we could change ASPECT's build system to define its own flags but so far is has worked well for us. For now we fixed the whole issue by adding NDEBUG in ASPECT's build system.

I think the major reason why we don't already do that is the simply reason that we don't use assert() internally so it hardly matters for us

Yeah I thought so. I just wanted to make sure it was not disabled on purpose for some reason.

Regarding NDEBUG - we could definitely set it for our release builds.

As long as there is no reason not to do it then I think it may be useful to set it just to avoid deviating from cmake standard values. Would the way I implemented it here be correct?

[drwells]

As long as there is no reason not to do it then I think it may be useful to set it just to avoid deviating from cmake standard values.

That's a good point - I am now convinced that we should do this. The way this patch sets NDEBUG is equivalent to how we set DEBUG in debug mode so I believe this is the correct approach.

[tamiko]

Good to go! (some minor rewording request).

[gassmoeller]

I modified the comments. Let's see what the testers say.

[tamiko]

/rebuild

[bangerth]

I regret that I chose DEBUG at the time (probably late 1997, early 1998) instead of the negative NDEBUG. The regret was instantaneous when I learned a few years later about NDEBUG, which I had simply not known about at the time.

It makes sense to me that we either set DEBUG or NDEBUG and that the presence of one implies the absence of the other. As for WorldBuilder, my suggestion would be to put this into its config.h file (assuming it has such a file):

  #ifdef DEAL_II_VERSION_MAJOR
  #  ifndef DEBUG
  #     define NDEBUG
  #  endif
  #endif

Since config.h should always be the first file #included, this will have the equivalent effect to actually setting NDEBUG, and it's something that can be done right now already.

I think these two things are independent.