CGAL: Deal with CGAL_assertion(false)

[afabri]

Summary of Changes

Replace or remove some CGAL_assertion(false) where it is sure that the code is unreachable, regardless the input. This is typically a switch for three enum values with 3 case statements that all return and where we have verified.

Release Management

• Affected package(s):
• Issue(s) solved (if any): fix #8295
• License and copyright ownership: unchanged

[mglisse]

Actually, I am not so sure with should replace CGAL_assertion(false) with CGAL_unreachable() everywhere. For users activating the CGAL assertions even in production, the code will behave differently.

Oh, I always assumed that CGAL_unreachable() was the same as CGAL_assume(0), i.e. that when assertions are enabled it would error out, but I now see this isn't the case. Was this a deliberate choice, or an accident?

[lrineau]

Actually, I am not so sure with should replace CGAL_assertion(false) with CGAL_unreachable() everywhere. For users activating the CGAL assertions even in production, the code will behave differently.

Oh, I always assumed that CGAL_unreachable() was the same as CGAL_assume(0), i.e. that when assertions are enabled it would error out, but I now see this isn't the case. Was this a deliberate choice, or an accident?

I am not sure it was intended. The commit that introduced that behavior is https://github.com/CGAL/cgal/pull/6052/commits/604d2fd1ffcd0072958ba53b606d80fe7b993883. Actually, I think it was intentional. The goal was to convince the compiler that the branch was dead, to remove warnings.

That means CGAL_unreachable() no longer triggers the CGAL::Assertion_exception exception, if CGAL assertion are enabled. And CGAL_assertion(false) is not equivalent to CGAL_unreachable().

CGAL_unreachable() should be restricted to cases where it is impossible the branch can be reached. Or if invalid user-inputs are already checked by other assertions. Otherwise, in Debug mode, users might get segfaults instead of CGAL assertion failures.

[mglisse]

Does that mean we should use CGAL_assume(false) by default, and only use CGAL_unreachable() in a few "safe" cases?

[afabri]

Concerning CGAL::assume() it is unlucky that it has not the same semantics as [[ assume ]].
We have probably not many users of CGAL::assume() so do we want to break it or do we want to a add a CGAL::cpp23::assume()? Or simply not use something like [[ assume ]]`?

[lrineau]

Concerning CGAL::assume() it is unlucky that it has not the same semantics as [[ assume ]]. We have probably not many users of CGAL::assume() so do we want to break it or do we want to a add a CGAL::cpp23::assume()? Or simply not use something like [[ assume ]]`?

[!NOTE] The name is CGAL_assume, because it has to be a preprocessor macro.

[afabri]

@sloriot this is no longer WIP.

[sloriot]

Successfully tested in CGAL-6.0.1-Ic-345

[afabri]

@sloriot what has to be approved?

[afabri]

What I don't like with CGAL_unreachable() replacement is that if there is a bug in CGAL code and that we reach a place marked as unreachable no assertion will be thrown. Maybe we should modify the macro to also raise CGAL_assertion(false).

The whole point is to tell the compiler that it can go ahead and optimize things. For example for the three orientation switch.

[sloriot]

What I don't like with CGAL_unreachable() replacement is that if there is a bug in CGAL code and that we reach a place marked as unreachable no assertion will be thrown. Maybe we should modify the macro to also raise CGAL_assertion(false).

The whole point is to tell the compiler that it can go ahead and optimize things. For example for the three orientation switch.

I know, what I'm saying is that the doc of _builtin_unreachable says that it's undefined behavior if the program "reaches" it. So without an assertion, we can't detect it.

[github-actions[bot]]

This pull-request was previously marked with the label Tested, but has been modified with new commits. That label has been removed.

[sloriot]

Successfully tested in CGAL-6.1-Ic-123