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
Avoid undefined behavior warning #14283
Conversation
/rebuild |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. I think we can go through this file again at a later point and see if there are things we might want to unify. (In the end, it is also a question about what we want to happen in the invalid case without assertions.)
@@ -1007,7 +1007,7 @@ ReferenceCell::n_lines() const | |||
return 12; | |||
|
|||
Assert(false, ExcNotImplemented()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assert(false, ExcNotImplemented()); | |
AssertThrow(false, ExcNotImplemented()); |
Shall we use an AssertThrow
here to make sure that this part is never reached in both debug
and release
mode? What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't use at such places AssertThrow
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree; while we expect the code to not be reachable, the compiler will not know that and still generates the code also in release mode. Having a smaller function content makes the likelihood of the compiler to inline the function higher, making it a win overall. We have the debug mode for ensuring consistency of data.
closes #14263
@kronbichler @peterrum: This was the only place, where the warning popped up for now.