-
Notifications
You must be signed in to change notification settings - Fork 168
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
ci: Enable standard library assertions in debug builds #3759
base: main
Are you sure you want to change the base?
Conversation
Maybe we should put this behind an option and enable it explicitly in the CI? That way we wouldn't silently modify local I'm definitely uncomfortable with adding this to |
Hmm I added it also for clang... but actually, I do not check which stdlib is used. Maybe we should add both macros unconditionally of the Regarding the option, you mean to avoid rebuild after this PR? |
@benjaminhuth I don't think we should unexpectedly enable any assertions outside of the CI, even in |
Okay yeah that makes sense, |
But just out of interest @paulgessinger: To me every standard library assertion would be a violated precondition, so what would be the issue with enabling them for GCC documentation even recommendes them for production builds actually (https://gcc.gnu.org/wiki/LibstdcxxDebugMode) |
📊: Physics performance monitoring for 1038fcbFull contents |
@benjaminhuth I don't see it being recommended, just that it's suitable to run in production. It's a HUGE change to enable e.g. bounds checking quietly. ATLAS builds with |
Yeah I agree that its a huge change, but would be a good change in my opinion. But yeah, I wasn't aware that by default acts is build in RelWithDebInfo for production builds in ATLAS. Thats of course a different story then... |
Sure, but there's no way we can decide this as a library. We can test with these assertions on (which is a very good idea imo), and experiments can then decide if they want them, but there's no way we can force this. |
Also this does not come for free right? I guess the standard lib functions will do extra checks which are skipped otherwise. So it is a bit like with our assertions while ours might be more expensive |
@andiwand I believe these are reasonably light checks, like bounds checks or smart pointer |
Seems like the detray conversion code does some out-of-bounds access, which ostensibly with these flags are checked at compile-time.
|
I'll go into this later/next week |
Right but ultimately this is something which should be measured before being deployed anywhere |
Should add assertions that check preconditions of calls to standardlibrary functions.