Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
lrineau opened this issue Jun 19, 2024 · 33 comments · May be fixed by #8496
Open

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

lrineau opened this issue Jun 19, 2024 · 33 comments · May be fixed by #8496
Labels

Comments

@lrineau
Copy link
Member

lrineau commented Jun 19, 2024

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
Copy link
Member

afabri commented Aug 6, 2024

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

@afabri
Copy link
Member

afabri commented Sep 24, 2024

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
Copy link
Member

afabri commented Sep 25, 2024

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

@afabri
Copy link
Member

afabri commented Sep 25, 2024

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
Copy link
Member

afabri commented Sep 25, 2024

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

@sloriot
Copy link
Member

sloriot commented Sep 25, 2024

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

assertion should be removed

@afabri
Copy link
Member

afabri commented Sep 25, 2024

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

@afabri
Copy link
Member

afabri commented Sep 25, 2024

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

@sloriot
Copy link
Member

sloriot commented Sep 25, 2024

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

I would remove it

@afabri
Copy link
Member

afabri commented Sep 25, 2024

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
Copy link
Member

sloriot commented Sep 25, 2024

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 afabri linked a pull request Sep 25, 2024 that will close this issue
@afabri
Copy link
Member

afabri commented Sep 25, 2024

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
Copy link
Member

afabri commented Sep 25, 2024

Shall we declare this copy constructor private instead?

@sloriot
Copy link
Member

sloriot commented Sep 25, 2024

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
Copy link
Member

afabri commented Sep 25, 2024

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
Copy link
Member

sloriot commented Sep 25, 2024

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
Copy link
Member

afabri commented Sep 25, 2024

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
Copy link
Member Author

lrineau commented Sep 25, 2024

Shall we declare this copy constructor private instead?

Make it = delete; instead.

@lrineau
Copy link
Member Author

lrineau commented Sep 25, 2024

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
Copy link
Member Author

lrineau commented Sep 25, 2024

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

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

@afabri
Copy link
Member

afabri commented Sep 26, 2024

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

@afabri
Copy link
Member

afabri commented Sep 26, 2024

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
Copy link
Member

sloriot commented Sep 26, 2024

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
Copy link
Member

afabri commented Sep 26, 2024

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
Copy link
Member

afabri commented Sep 26, 2024

@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
Copy link
Member

sloriot commented Sep 27, 2024

@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
Copy link
Member

afabri commented Sep 27, 2024

@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
Copy link
Member

afabri commented Sep 27, 2024

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

@MaelRL
Copy link
Member

MaelRL commented Sep 27, 2024

@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
Copy link
Member

MaelRL commented Sep 27, 2024

@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
Copy link
Member

afabri commented Sep 27, 2024

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

@afabri
Copy link
Member

afabri commented Sep 27, 2024

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

@gdamiand
Copy link
Member

@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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants