-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Implement From<TryReserveError>
for io::Error
#96505
Conversation
The `try_reserve` methods may be called when allocating for I/O.
Hey! It looks like you've submitted a new PR for the library teams! If this PR contains changes to any Examples of
|
r? @m-ou-se (rust-highfive has picked a reviewer for you, use r? to override) |
r? rust-lang/libs-api @rustbot label +T-libs-api -T-libs |
I feel somewhat similar about this as I did about #96104, although mapping TryReserveError to io::ErrorKind::OutOfMemory seems more obvious and universally correct. I wonder if CapacityOverflow should map to OutOfMemory as well, instead of InvalidInput. CapacityOverflow usually means you're trying to allocate even more than could possibly be allocated, which arguably still fits within the 'out of memory' category. |
TryReserveErrorKind::AllocError { .. } => ErrorKind::OutOfMemory, | ||
}; | ||
|
||
Error::new(kind, error) |
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.
Note that this involves a double allocation, effectively a Box<Box<dyn io::Error>>
.
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.
I didn't realize io::Error::new
was a double-allocation! I wonder if thin dyn box would solve that.
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.
Even worse: if you pass it a string literal ( Error::new(kind, "...")
) you get a triple allocation (and a strcpy/memcpy) as the inner box will contain a String
.
ThinBox can save one allocation here. I think that'd be a simple change to make, but there might be some non-obvious pitfall.
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.
See #83352
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.
Since you agree with #96104 (comment), the same point definitely applies to this impl too so I'll close the PR. Thanks anyway!
The
try_reserve
methods may be called when allocating for I/O.