@GilesBathgate Could you please rebase your branch from cgal/master?

@sloriot I find the patch good and useful. Could you also have a look?

TODO: I think that however the pull-request should also modify the documentation. The file to modify is Documentation/doc/Documentation/Developer_manual/Chapter_checks.txt

Instead of disabling totally the macro, could it be better to display a message about the problem when std::uncaught_exception() ?

@gdamiand Although this does disable the macro from throwing an exception when the stack is unwinding, the uncaught exception still propagates, and so you still get an exception message which can be handled gracefully. If you are saying that the uncaught exception should be caught within the macro, some additional information added to it, and then rethrown. I guess that might work.

Sorry, but I don't understand how this works.

If I am in a destructor and I use the new macro CGAL_destructor_assertion(EX), then std::uncaught_exception() is true ?

If yes, then the CGAL_destructor_assertion macro is disabled and no assertion are raised.

If no, assertion is raised like previously.

@gdamiand There are two scenarios.

Scenario A: No exceptions were thrown by the class, but the assertion in the destructor is false. In this case it is fine to throw an exception from the destructor.

Scenario B: The class threw an exception in a method. As part of the exception clean-up routines the program unwinds the stack, and deletes local variables as it does so. When it tries to delete the class that also fails the assertion in its destructor, it is now in a situation where the current exception has not been caught, (and there is no chance of catching it the destructor either because it exists on different stack frame). At this point if you throw another exception the program terminates, (via std::terminate())

I admit its a little tricky to grasp,I found this article quite useful: http://www.kolpackov.net/projects/c++/eh/dtor-1.xhtml

The example given here http://en.cppreference.com/w/cpp/error/uncaught_exception also illustrates the scenarios I described above.

@gdamiand Here is a demonstration program, the program demonstrates Scenario A and that part works as expected. But in Scenario B it should also print "Exited program cleanly" but it doesn't because of the uncaught exception (uncommenting the line in the destructor will fix the problem)

#include <iostream>
#include <exception>
#include <stdexcept>

class Foo {
int check_protocoll;
public:
    Foo() : check_protocoll(0){}
    ~Foo() {
        // if(!std::uncaught_exception())
             if(check_protocoll) throw std::runtime_error("Destructor assertion failed");
    }

    void success()
    {
        check_protocoll=1;
    }

    void fail() {
       check_protocoll=1;
       throw std::runtime_error("Assertion failed");
    }
};

int main()
{
    // Scenario A
    try {
        Foo f;
        f.success();
    } catch (const std::exception& e) {
        std::cout << "Exception caught: " << e.what() << '\n';
    }
    // Scenario B
    try {
        Foo f;
        f.fail();
    } catch (const std::exception& e) {
        std::cout << "Exception caught: " << e.what() << '\n';
    }
    std::cout << "Exited program cleanly\n";
}

OUTPUT

Exception caught: Destructor assertion failed
terminate called after throwing an instance of 'std::runtime_error'
  what():  Destructor assertion failed
Aborted (core dumped)

Output when line uncommented

Exception caught: Destructor assertion failed
Exception caught: Assertion failed
Exited program cleanly

Ok, I understand now. Thank you very much for the detailed answer.

It may also be worth noting that when the above program is compiled for c++11, even Scenario A terminates the program. This is because in c++11 you have to explicitly mark destructor's with noexcept(false), if they are expected to throw exceptions.

@lrineau I propose an additional macro CGAL_NOEXCEPT(false), which will only be defined when CGAL_CXX11 is defined. This is to take care of the situation in my previous comment. Should I put that in another merge request or add it to this one?

@GilesBathgate You can add it to this pull-request. It should also be documented in the developers manual.

@lrineau I am trying to put together some notes for the documentation. This is a first draft, but I am not an excellent technical documenter.

...

• Destructor assertions These are checks which can be made within normal clean up of an object. A special macro is provided to ensure these checks are not made when the object is being cleaned up during exception handling.

The according macro names all have the format CGAL_<check_type> where <check_type> can be one of

• precondition
• postcondition
• assertion
• warning
• static_assertion
• destructor_assertion

...

Exception handling

Any destructor which might throw an exception, including a destructor which uses the destructor_assertion macro, should be marked with the CGAL_NOEXCEPT(false) macro. This macro provides future compatibility with C++11 and above which provides the noexcept keyword.

@GilesBathgate, that looks good. I suggest a small modification: explicit the name of the special macro.

A special macro CGAL_destructor_assertionis provided to ensure...

@lrineau I have updated the documentation in this merge request now. Is there anything else you would like doing before lifting the TODO flag?

@GilesBathgate Nothing, as far as I can see. It looks good.

@sloriot will now merge your pull-request in our testsuite branch, to test if it can be integrated in cgal/master.

Merged in CGAL-4.8-Ic-65

Unfortunately, for the moment that web site is password-protected. We plan to migrate to a Docker container for it, and then we will open it to public.

Why not CGAL_NOEXCEPT(CGAL_NO_ASSERTIONS), to avoid penalizing release builds?

@mglisse Sounds sensible, I assume you mean to amend calls to the macro, rather than the macro itself?

@lrineau Shall I push this change also, or does this interfere with the current test cycle?

@GilesBathgate You can push a modification. It will be retested.

Merged in CGAL-4.8-Ic-91

CGAL_NO_ASSERTIONS is a macro keyword that is defined or not. That is you cannot use it as a boolean. You can add in Installation/include/CGAL/config.h:

    #ifndef CGAL_NO_ASSERTIONS
        #define CGAL_NO_ASSERTIONS_BOOL 0
    #else
       #define CGAL_NO_ASSERTIONS_BOOL 1
    #endif

and use it when using CGAL_NOEXCEPT.

That would be simpler to define CGAL_NO_ASSERTIONS to 1, when it is defined.

But then you're breaking the former usage of CGAL_NO_ASSERTIONS

and this will not solve the problem if it is not defined...

How would it be broken?! As long as the macro is never defined to 0, that makes no difference if CGAL_NO_ASSERTIONS is defined to 1 or to the empty string.

@lrineau Because I use CGAL_NO_ASSERTIONS in place of false in commit GilesBathgate@0b11d8d
e.g. CGAL_NOEXCEPT(CGAL_NO_ASSERTIONS) as suggested by @mglisse

@lrineau Furthermore I don't think an undefined macro evaluates to an empty string, it would give the compiler error: ‘CGAL_NO_ASSERTIONS’ was not declared in this scope

Currently in CGAL-4.7, the macro is either undefined, or defined to an empty string:

#define CGAL_NO_ASSERTIONS

You can use an undefined macro in #ifdef/#ifndef, but also in #if: in conditional preprocessing directives, any unknown identifier is replaced by 0 before the expression is evaluated. But that is the only place I know where you can use an undefined macro (undefined identifier). And that is not true for a macro that is defined to the empty string. With the following code:

#define A
#if A
#endif

the compiler returns an error:

test.cpp:2:6: error: #if with no expression
 #if A
      ^

That is why I would like that, in CGAL, we define macro that way:

• either the macro is left undefined,
• or it is defined to 1.

But, indeed, that would not be sufficient for you use CGAL_NOEXCEPT(CGAL_NO_ASSERTIONS).

@lrineau Thanks that makes it clear. I think @sloriot solution is the cleanest in this situation.

Merged in CGAL-4.8-Ic-99