-
Notifications
You must be signed in to change notification settings - Fork 77
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
Replace a panic with an error branch #544
Conversation
Question: is
|
Question: This branch is already out-of-date and needs changes, do you prefer that I do a merge or that I do a rebase? |
Rebase please, thanks! |
885dacb
to
e08b742
Compare
Maybe we could also enable Also, we'll probably want some way of disabling these in testing code, since we do want to be able to unwrap and expect in tests. Maybe we can just put these behind a |
e08b742
to
2b6b0f1
Compare
Good point. There is already a |
2b6b0f1
to
24f8dbb
Compare
24f8dbb
to
9dc3496
Compare
9dc3496
to
c869b60
Compare
c869b60
to
41d6bc6
Compare
23eea59
to
97d850e
Compare
Updated statistics for We have one function that is documented as panicing as part of its public API ( We have one function that called We have four functions ( One useful warning versus five useless warnings is not a great ratio. I'll let @joshlf decide whether you think that |
IIUC these will be resolved by #368, at least for byte slice types (smart pointer types like
I'm leaning no, which is a shame because you've put a lot of work into this PR, but I agree that it's a lot of noise for not much benefit. That said, maybe you could land the refactor of |
aab9374
to
d13395c
Compare
clippy::expect_used
, address warnings
Not really -- most of the work on this PR has been on further refining what exactly it means to "prevent panics" in zerocopy, and what sort of tradeoffs we want to make in achieving that goal. I decided to stick with one-Clippy-lint-per-PR so that "we don't want lint X" is an easy decision to make.
I followed the "simplify this PR" route so it only lands that one change, and renamed the PR accordingly. |
d13395c
to
4c1741d
Compare
4c1741d
to
e701287
Compare
e701287
to
5d6c8ce
Compare
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.
Sorry this has taken so long to get to.
Also, see #1125 and feel free to chime in there.
src/lib.rs
Outdated
.copy_from_slice(self.as_bytes()); | ||
// get_mut() should never return None here. We use ? rather than | ||
// .unwrap() because in the event the branch is not optimized away, | ||
// returning None is generally lighter-weight than panicing. |
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.
// returning None is generally lighter-weight than panicing. | |
// returning None is generally lighter-weight than panicking. |
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.
Fixed.
This panic is unreachable. Panicxing is heavyweight, so we're trying to reduce panics (issue google#202). This PR replaces the panic with a branch that should be lighter-weight (if it is not optimized away entirely).
5d6c8ce
to
52878cc
Compare
This enables Clippy's
expect_used
lint, which errors on calls toOption::expect
andResult::{expect, expect_err}
. The intent of enabling this lint is to reduce the number of panics emitted inzerocopy
's code (issue #202).