-
Notifications
You must be signed in to change notification settings - Fork 392
Fix misplaced assert statement causing a lot of warnings
#2988
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
Conversation
terhorstd
left a comment
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.
minor change
gtrensch
left a comment
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.
Approved.
assert statement causing a lot of warnings
heplesser
left a comment
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.
@muffgaga Thanks for this fix! Which compiler and which compiler options do you use? I have not seen this warning before, so our warning settings are apparently not strict enough. Also, even though the limited "Schaffungshöhe" of this PR does not strictly make it necessary, would you send us a contributor agreement (https://nest-simulator.readthedocs.io/en/latest/_downloads/9b65adbdacba6bfed66e68c62af4e308/NEST_Contributor_Agreement.pdf)?
It was the EBRAINS Software environment/Collab one — i.e. gcc@10.3.0, and "Release" build was mentioned in the build log (see #2993 for a build issue using the same compiler/software environment). (Maybe
Yes, will do. |
→ sent to @terhorstd (he asked me directly via chat). |
In release builds, the assert is omitted and the function shows undefined behavior due to not returning. We now return nullptr to avoid UB; in general we expect developers to build in non-release mode, and to encounter the assert when code wrongly dispatches to this function.
Co-authored-by: Hans Ekkehard Plesser <hans.ekkehard.plesser@nmbu.no>
|
@muffgaga Thanks a lot, will merge now! |
|
@terhorstd Since you had previously requested changes to this PR, you need to approve it now :). |
terhorstd
left a comment
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.
LGTM 👍
Thanks!
I just saw this warning quite often (in the EBRAINS spack build it gets repeated for 162 times ;) ):
Also: I guess even if the
assertis ensured to never be removed (contrary to standardNDEBUGbehavior) it would be reasonable to clearly state the intention here (an exception might be more user-friendly, but I guess the code was intentionally avoiding an exception) by calling abort?