Please allow -DNDEBUG and CGAL_nnn_assertions at the same time

[GilesBathgate]

Issue Details

There seems to be some very confusing and inconsistent usage of the NDEBUG define within the CGAL library.

Source Code

It seems the correct way to disable CGAL_NEF_TRACE.* is to define NDEBUG : https://github.com/CGAL/cgal/blob/869833136d6e2b0487e8d083f55fae7afd151258/Nef_2/include/CGAL/Nef_2/debug.h#L44-L50 but then all the assertions generated from script cgal_create_assertions.sh, we need to not define NDEBUG or the assertions will be disabled: https://github.com/CGAL/cgal/blob/869833136d6e2b0487e8d083f55fae7afd151258/STL_Extension/include/CGAL/triangulation_assertions.h#L30-L31

Furthermore in: https://github.com/CGAL/cgal/blob/869833136d6e2b0487e8d083f55fae7afd151258/Nef_3/include/CGAL/Nef_3/K3_tree.h#L527-L539 Defining NDEBUG would exclude some assertions that we potentially do want, meanwhile in: https://github.com/CGAL/cgal/blob/869833136d6e2b0487e8d083f55fae7afd151258/Nef_3/include/CGAL/Nef_3/SNC_external_structure.h#L663-L688 Trace code is yet again included that isn't desired, in release builds.

This leaves me wondering if I am not supposed to include CGAL_assertions in release builds at all, or is the developer expected to use #include hacks like this:

https://github.com/openscad/openscad/commit/bb0ec94290733835df0716531c30460fc5b5210a

Or this:

https://github.com/GilesBathgate/RapCAD/commit/bedd95e3f12488f503c715e81c3a9d32fb1a3cf0

Would it not be better to have something like CGAL_USE_TRACE that can be defined if the trace code is desired?

Finally CGAL_DEBUG supposedly allows forcing CGAL assertions, even if NDEBUG but this rule doesn't seem to follow in the assertions generated by the script cgal_create_assertions.sh

Environment

• Operating system (Windows/Mac/Linux, 32/64 bits): Linux
• Compiler: gcc
• Release or debug mode: Release
• Specific flags used (if any): NDEBUG
• CGAL version: 5.0.2
• Boost version: 1.71.0
• Other libraries versions if used (Eigen, TBB, etc.): Qt 5.12.8

[lrineau]

The standardized semantic of NDEBUG is its effect on assert: the definition of the NDEBUG macro disables standard-C assertions. It meaning is probably "no debug".

[GilesBathgate]

@lrineau I do agree with this definition, but I think it would be useful to be able to either (or both):

1) Disable tracing when NDEBUG is not defined. 2) Enable CGAL_assertions when NDEBUG is defined (i.e what CGAL_DEBUG says it forces in assertions.h)

Should I open PR or Feature request for these?

[mglisse]

Indeed, some things look like they could be improved. (CGAL_)NDEBUG should only control assertions, not printing traces (probably some developer misunderstood it for "whatever code I use when I debug"). Disabling assertions is the whole point of NDEBUG. NDEBUG disables all assertions, CGAL_NDEBUG disables assertions in CGAL without disabling assertions in the user's code. Release builds are expected not to have assertions in general. Protecting CGAL_assertion_code with #ifndef NDEBUG doesn't make sense, just drop the #if. Doesn't defining both NDEBUG and CGAL_DEBUG already do what you want for 2. ?

[GilesBathgate]

Doesn't defining both NDEBUG and CGAL_DEBUG already do what you want for 2. ?

@mglisse Unless i've misunderstood, this will disable the assertions in triangulation_assertions.h because of the || defined(NDEBUG)

[mglisse]

I think you could just drop the || defined(NDEBUG), it is already visible through CGAL_NO_ASSERTIONS unless some other flag counteracts it.

[GilesBathgate]

@mglisse That seems right yes.

https://github.com/CGAL/cgal/blob/869833136d6e2b0487e8d083f55fae7afd151258/STL_Extension/include/CGAL/assertions.h#L26-L45

So if we define NDEBUG, then CGAL_NDEBUG will be defined, which would then define CGAL_NO_ASSERTIONS, CGAL_NO_PRECONDITIONS, CGAL_NO_POSTCONDITIONS, CGAL_NO_WARNINGS meaning there is no need for any || defined(NDEBUG) in any of those files check for that.

The only thing that makes me weary is the comment: https://github.com/CGAL/cgal/blob/869833136d6e2b0487e8d083f55fae7afd151258/Scripts/scripts/cgal_create_assertions.sh#L49-L51

[mglisse]

Indeed, that comment seems to indicate that this is done on purpose. It seems wrong though. If you only want to change the behavior for triangulations between the inclusion of several triangulation headers (that's an awfully specific need and rather fragile, we don't document which ones include which others), you should define the more specific macro. If you want a more general change, the multiple-inclusion trick should also be done in the general assertions.h. But then I have never used those assertion possibilities beyond CGAL_NDEBUG, it would be better to have the opinion of people who actually use them, assuming they exist.

[lrineau]

@GilesBathgate I agree with what was said so far. I suggest to summarize your thoughts by a change of the issue title: "Please allow -DNDEBUG and CGAL assertions at the same time".

[lrineau]

@lrineau #5245 was also part of the fix.

You marked https://github.com/CGAL/cgal/pull/5246 with fix #5240, and that is a standard behavior of Github to close "linked issues" once a pull-request is merged (see https://docs.github.com/en/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue).

I reopen.

(Anyway, I prefer that automatic behavior, plus reopening if necessary, than having fixed issues stay open in Github.)