-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
promote debug_assert to assert when possible and useful #99624
Conversation
(rust-highfive has picked a reviewer for you, use r? to override) |
Hey! It looks like you've submitted a new PR for the library teams! If this PR contains changes to any Examples of
|
This comment has been minimized.
This comment has been minimized.
58b239c
to
9c5fc67
Compare
@bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit 9c5fc6726f5744deafd45eb4dda5b1ab52cfa8bd with merge f5809960996c3d9abb4b32698a720c41a619be38... |
☀️ Try build successful - checks-actions |
Queued f5809960996c3d9abb4b32698a720c41a619be38 with parent 93ffde6, future comparison URL. |
Finished benchmarking commit (f5809960996c3d9abb4b32698a720c41a619be38): comparison url. Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)Results
CyclesResults
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. @bors rollup=never Footnotes |
4571565
to
a278a74
Compare
cd128aa
to
c8c04e2
Compare
r? @Amanieu for the interaction with panic-in-Drop = abort -- there's a few asserts here that are in Drop impls. |
I would prefer keeping these as Since these asserts will never fire in practice, panic-in-drop is irrelevant here. |
⌛ Testing commit 6231d17513119ffbb2d7c4724b2fa28311e32fc5 with merge 548063328dc5e6d2435446a46dd13d7e85666777... |
💔 Test failed - checks-actions |
This comment has been minimized.
This comment has been minimized.
04469b2
to
3dddcbe
Compare
@rustbot label -S-waiting-on-author +S-waiting-on-review |
This comment has been minimized.
This comment has been minimized.
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
3dddcbe
to
d91dff3
Compare
@bors r+ |
☀️ Test successful - checks-actions |
That's the thing, just because POSIX doesn't list an error doesn't mean the function cannot raise the error -- error lists are not exhaustive. See #94705. |
I agree with you @RalfJung I do not care if the docs tell us that this will not fail at all, a sanity check is always good. However the majority of the review point in the way to do not add this promotion assert! I'm open to discuss it, but I think before reopen the issue we need to converge on trusting the POSIX docs or not otherwise work on this simple change will be super confusing because i like a ping pong game you add and revert the change! |
Finished benchmarking commit (569788e): comparison url. Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)Results
CyclesResults
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. @rustbot label: -perf-regression Footnotes |
The issue should remain open until we decided for good one way or the other, but I agree that things should be discussed on the issue before another PR is opened. |
Maybe it is good to create a zulip thread for this |
This PR fixed a very old issue #94705 to clarify and improve the POSIX error checking, and some of the checks are skipped because can have no benefit, but I'm sure that this can open some interesting discussion.
Fixes #94705
cc: @tavianator
cc: @cuviper