-
Notifications
You must be signed in to change notification settings - Fork 370
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
Fix misplaced assert
statement causing a lot of warnings
#2988
Conversation
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
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
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. [email protected], 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 <[email protected]>
@muffgaga Thanks a lot, will merge now! |
@terhorstd Since you had previously requested changes to this PR, you need to approve it now :). |
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
assert
is ensured to never be removed (contrary to standardNDEBUG
behavior) 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?