CGAL_assertion(false)/CGAL_error() replace by CGAL_unreachable()?

[lrineau]

There are more than 250 occurrences of CGAL_assertion(false) in CGAL. And other occurrences of CGAL_error(). Should not we replace them with CGAL_unreachable()?

[afabri]

Is there really something to discuss? I guess it must be dealt with case by case and cannot be a global replace.

[afabri]

After an assertion we in general still return something in case the function has a return value. Is this also the case when replacing with the unreachable statement?

[afabri]

Can a default: in a switch be at the very beginning (see here)? Does it then still go to the other cases?

[afabri]

Do you agree that we make this to a CGAL_assertion_msg()?
With assertions switched off, we then no longer see something on cerr, but that is not desired anyways.

[afabri]

Do you agree that here we cannot replace because it is something that can happen ?

[sloriot]

Do you agree that here we cannot replace because it is something that can happen ?

assertion should be removed

[afabri]

Does it make sense to have an assertion in TDS_3::is_valid()?

[afabri]

Shall we make here the last case to default and then we don't need an assert or unreachable statement.

[sloriot]

Does it make sense to have an assertion in TDS_3::is_valid()?

I would remove it

[afabri]

Do you agree that here we cannot replace because it is something that can happen ?

assertion should be removed

So unreachable means that one may still arrive there in practice?

[sloriot]

Do you agree that here we cannot replace because it is something that can happen ?

assertion should be removed

So unreachable means that one may still arrive there in practice?

In this case, it emits an assertion while returning false is the expected behavior.

[afabri]

Do you agree that here we cannot replace because it is something that can happen ?

assertion should be removed

So unreachable means that one may still arrive there in practice?

In this case, it emits an assertion while returning false is the expected behavior.

But when we may arrive at that point, we maybe want to get an exception thrown by a custom error handler.

[afabri]

Shall we declare this copy constructor private instead?

[sloriot]

Do you agree that here we cannot replace because it is something that can happen ?

assertion should be removed

So unreachable means that one may still arrive there in practice?

In this case, it emits an assertion while returning false is the expected behavior.

But when we may arrive at that point, we maybe want to get an exception thrown by a custom error handler.

you can assume it!=end since there is a check before

[afabri]

Would this also compile if the static function here was just removed? After all we need the class only because of the offered specializations.

[sloriot]

Would this also compile if the static function here was just removed? After all we need the class only because of the offered specializations.

Indeed, you should get a compilation error instead of a runtime error.

[afabri]

Would this also compile if the static function here was just removed? After all we need the class only because of the offered specializations.

Indeed, you should get a compilation error instead of a runtime error.

To make it very clear we could even declare the constructor =0.

[lrineau]

Shall we declare this copy constructor private instead?

Make it = delete; instead.

[lrineau]

Would this also compile if the static function here was just removed? After all we need the class only because of the offered specializations.

I agree it should be removed. Once it is remove, if a user-code uses Get_beta<Face_graph, i> for i>3, there would be a compilation error.

[lrineau]

There should be only a declaration of the template, without any definition. Just:

template<typename HEG, unsigned int i> struct Get_beta;

[afabri]

Here we have the assertion as first line of a function.

[afabri]

Is there a subtlety I miss here which makes that there is a CGAL_assertion(false) at the end, while before we have several CGAL_unreachable() @sloriot

[sloriot]

Here we have the assertion as first line of a function.

git blame says that the code is 16yo and from Peter. So basically it was always there. We can remove the function.

[afabri]

Here we have the assertion as first line of a function.

git blame says that the code is 16yo and from Peter. So basically it was always there. We can remove the function.

Well, it is called here.

[afabri]

@sloriot any idea what the function initialize_index_map() is about? I do not see it used anywhere. Do we have it for backward compatibility?

[sloriot]

@sloriot any idea what the function initialize_index_map() is about? I do not see it used anywhere. Do we have it for backward compatibility?

probably a leftover after some refactoring.

[afabri]

@MaelRL here I would prefer to see added the case T::NONE which is the default. Can it by construction not be NONE? Then I will put CGAL_unreachable()

[afabri]

@MaelRL does "meaningless" here that this cannot happen for whatever input, or does it mean that the user called locate() with illegal input?

[MaelRL]

@MaelRL does "meaningless" here that this cannot happen for whatever input, or does it mean that the user called locate() with illegal input?

illegal input, this is a violation of the precondition:

/// \pre `loc` corresponds to a point that lies on a face incident to both `loc.first` and `fd`.

[MaelRL]

@MaelRL here I would prefer to see added the case T::NONE which is the default. Can it by construction not be NONE? Then I will put CGAL_unreachable()

OK

[afabri]

@efifogel can you help me to understand the design idea for the generic operator here, which is supposed not to be used.

[afabri]

@gdamiand can you help me to understand the design idea for the function run() here , which is supposed not to be used.

[gdamiand]

@gdamiand can you help me to understand the design idea for the function run() here , which is supposed not to be used.

The functor Test_split_nonvoid_attribute_functor_run<CMap, i, j> tests if i-attributes are split after an operation, except for j-attributes.

As said in the comments, for j==0 or j==1, we use only the version with two lists of darts, thus this run method is never called (but should exist for the compiler).