CGAL
CGAL_assertion(false)/CGAL_error() replace by CGAL_unreachable()?
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()?
Is there really something to discuss? I guess it must be dealt with case by case and cannot be a global replace.
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?
Can a default: in a switch be at the very beginning (see here)? Does it then still go to the other cases?
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.
Do you agree that here we cannot replace because it is something that can happen ?
assertion should be removed
Shall we make here the last case to default and then we don't need an assert or unreachable statement.
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?
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.
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
falseis the expected behavior.
But when we may arrive at that point, we maybe want to get an exception thrown by a custom error handler.
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
falseis 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
Would this also compile if the static function here was just removed? After all we need the class only because of the offered specializations.
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.
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.
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.
There should be only a declaration of the template, without any definition. Just:
template<typename HEG, unsigned int i> struct Get_beta;
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
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.
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.
@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 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.
@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()
@MaelRL does "meaningless" here that this cannot happen for whatever input, or does it mean that the user called locate() with illegal input?
@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`.
