Implement ArrayVec and use it for RegisterRuleMap and UnwindContext #582
Conversation
There was a problem hiding this comment.
I'm happy to merge this because I think this is heading in the right direction, but do you think we'll need to make this ArrayVec implementation public in order for you to choose between Vec and ArrayVec for #581? If so, maybe we can revert back to the ArrayVec crate as a dependency instead, but make it optional and change the default here to Vec, since I think that's actually a better choice for many users.
I am still experimenting different designs and I am not so sure about whether this should be public. I feel that this is a worthy change about readability/maintainability so I send a PR anyway.
Wouldn't that break signal safety guarantee? |
nbdd0121
force-pushed
the
master
branch
3 times, most recently
from
6ac75dc
to
799ec15
Compare
src/read/util.rs
Outdated
| type Target = [A::Item]; | ||
|
|
||
| fn deref(&self) -> &[A::Item] { | ||
| let slice = &A::as_slice(&self.storage)[..self.len]; |
There was a problem hiding this comment.
We didn't previously bounds check this in RegisterRuleMap::rules or RegisterRuleMap::rules_mut. There is a performance regression for the benchmark cfi::iterate_entries_evaluate_every_fde, and removing this and the bounds check in deref_mut seems to fix that.
There was a problem hiding this comment.
What's the status of this PR? Do you think this bounds check is something that can be changed?
There was a problem hiding this comment.
Sorry I missed the comment. Will fix and push soon.
The default behaviour would, but users could use |
Essentially undoes #497, but provide an in-crate implementation of a minimal ArrayVec instead of using a dependency.
It should aid readability and keeping all
unsafein one place should make auditing easier.