Always disable assertions in release

[barche]

I was incorrectly assuming our release tests were free of assertions... it turns out this is not the case, due to CF_ENABLE_ASSERTIONS being always on unless explicitly set manually. I propose to directly set NDEBUG always on release.

This is related to commit d71fb9e8b5f1df88ece95abcc33365f3d1cec92d which was exposed by a clang build, and which made me wonder why it had ever worked on my other release builds.

[tlmquintino]

This is due to an error in the setting of the cmake options.

I dont agree that the fix should be setting NDEBUG directly.

It should work as a default incase the user does not set anything. It should set the default of CF3_ENABLE_ASSERTIONS as OFF when in Release and otherwise ON on other builds. There are also some other variables which have similar behavior. This lets the developer still override it.

I will fix this.

[barche]

It should be very severely discouraged, as in have no option for it, and if the developer REALLY wants NDEBUG defined for a Release build, he must know what he's doing, so he knows how to set a build flag manually from the CMake system. NDEBUG kills performance, I don't want people inadvertently producing production builds with it turned on.

[tlmquintino]

Yes, but this is what I meant. To be more clear:

• This defines a production build:

-DCMAKE_BUILD_TYPE=Release

• This defines a development build:

-DCMAKE_BUILD_TYPE=RelWithDebInfo

• This defines a very special production with assertions build:

-DCMAKE_BUILD_TYPE=Release -DCF3_ENABLE_ASSERTIONS=ON

• This defines a debug without assertions build (possibly for profiling):

-DCMAKE_BUILD_TYPE=RelWithDebInfo -DCF3_ENABLE_ASSERTIONS=OFF

The problem is that if you add the CXX_FLAG NDEBUG then it is very hard to remove it (have to parse/search the existing flags and remove that entry). The above seems to me clear, simple and powerful.

[barche]

Yes, and that is what the current code attempts, but it fails, because CF3_ENABLE_ASSERTIONS is on by default, and the build system won't override when switching to Release.

[barche]

OK, fixed it by adding a "CF3_EXTRA_DEFINES" option, where the user can override the defaults by adding things like -DNDEBUG or -UNDEBUG. There is also a utest that fails on unexpected assertion behavior.

[wdeconinck]

Why wouldn't the build system override CF3_ENABLE_ASSERTIONS? a caching issue?

Maybe the -DNDEBUG should always be disabled in Release builds (no option), but CF3_ENABLE_ASSERTIONS is a useful option, even in release builds, and it was clear from the print-summary that it is on or off. I think it should still be possible but ONLY enable our cf3_asserts, and nothing else.

Next question: What about stl-asserts, eigen-asserts and boost-asserts?

[barche]

Yes, it's a caching issue. You can use FORCE to override, but then the build system will always set it to what it wants. My commit from today allows all possible options for power users, and behaves as expected in debugging and release modes.

Our cf3_assert depends on the NDEBUG define, which is the same one that is used by boost and STL and almost everyone else. We could use another define, but I must stress that setting assertions to on for release must be kept difficult enough, and most certainly not the default as it was: the proto code runs 3 times as fast with assertions off now.

[tlmquintino]

I prefer to find a better (cmake) solution, if you don't mind.

Lets keep this CF3_EXTRA_DEFINES for power user additional defines (including NDEBUG), but try to correct the wrongful behavior described above. I had already set myself to look into this issue, so just give me some time.

[barche]

Sure, no problem, though I don't think it's that easy. Anyway, this is really academic: expected behavior is for NDEBUG to be defined in Release and to be not defined in Debug or RelWithDebInfo. At all times, this should be respected by default.

I think use cases for deviating from these defaults are extremely limited, if they even exist at all, so I wouldn't waste too much time on it.

[tlmquintino]

I suppose we will be learning some more cmake ...