Remove ArrayVec dependency #414
Conversation
|
CI failed with "error: Could not compile |
|
I'm personally preferring to use |
|
@Lokathor I've implemented Debug manually |
|
@jeehoonkang the selling point of Rust is that you can trust the compiler to verify safety for you instead of trusting humans to get it right. ArrayVec might be sound now (although I haven't checked, and you wouldn't believe what I've found in some other popular Rust crates recently), but having an unsafe dependency is not just about now, it's a continuous burden. Not only you have to audit all unsafe blocks in the the current version, you also have to commit to auditing all unsafe blocks in all future versions. Between taking on the burden of manually auditing all the unsafe in all future versions of ArrayVec and rewriting this small bit as safe code without even incurring a performance penalty, the choice is pretty obvious. |
| Deferred::new(no_op_func), Deferred::new(no_op_func), Deferred::new(no_op_func), Deferred::new(no_op_func), | ||
| Deferred::new(no_op_func), Deferred::new(no_op_func), Deferred::new(no_op_func), Deferred::new(no_op_func), | ||
| Deferred::new(no_op_func), Deferred::new(no_op_func), Deferred::new(no_op_func), Deferred::new(no_op_func), | ||
| Deferred::new(no_op_func), Deferred::new(no_op_func), Deferred::new(no_op_func), Deferred::new(no_op_func)] |
There was a problem hiding this comment.
I'm curious on two things over why are this many particular Deferred objects created here and is there a static way to populate the list than to hardcode each new obj construction (to make it DRY'er)?
There was a problem hiding this comment.
Because Deferred isn't copy, there is only the unsafe option to start with mem::uninitialized and then fill the array with ptr::write, but that would defeat the point of this PR. I don't believe macro_rules macros can pattern match on natural numbers (so that we could insert the same expression n times with a macro), and a compiler plugin would be overkill, so it seems we have to tolerate this until the mentioned rust-lang issue is resolved.
@Shnatsel could you mark this as TODO?
There was a problem hiding this comment.
You can match on a number but you're just matching on the token form of a number, so if you match on 3 you know that you're in the 3 branch but can't automatically use the 3 in any way. You'd need to write a macro that expanded each case by hand anyway.
There was a problem hiding this comment.
You could definitely do this with a macro, maybe just not in the most natural way. For example you could hardcode 0-10 fold repetition and write 64-fold repetition as repeating by 6 * 10 + 4 🙂
|
@jeehoonkang If there is no performance penalty (and I don't see why there should be one, as we'd still be using a staticly-allocated array) and we end up with fewer unsafe dependencies, I think the slightly ugly array initialization is acceptable, so I'd support merging this. |
|
Ugly array initialization marked TODO as requested. Is there anything else to be done, or is it ready for merging? |
| for deferred in self.deferreds.drain(..) { | ||
| for i in 0..self.len { | ||
| let no_op = Deferred::new(no_op_func); | ||
| let deferred = mem::replace(&mut self.deferreds[i], no_op); | ||
| deferred.call(); | ||
| } |
There was a problem hiding this comment.
This can be an iteration over &mut self.deferreds[..self.len]
442: Do not perform superfluous bounds checks on dropping crossbeam-epoch::Bag r=jeehoonkang a=Shnatsel Addresses [this comment](#414 (comment)) that I didn't get a chance to act upon before merging #414. This way bounds checks should be performed only once. However, this does not measurably impact benchmarks. Co-authored-by: Sergey "Shnatsel" Davidoff <[email protected]>
…ds when checking server token. Fixes crossbeam-rs#414
ArrayVecis used in just one place, and even there crossbeam uses a small part of its API surface. This PR replaces ArrayVec with 100% safe code using a plain old array and ditches the ArrayVec dependency altogether.This removes 170 unsafe expressions from the dependency tree according to cargo-geiger. With less unsafe code to worry about there's less chance of memory safety bugs, and safety/security audits will be cheaper and easier to perform.
An unfortunate casualty of this change is
#[derive(Debug)]on internal structures because there is no Debug implementation for[T; 64]- these impls are only generated up to length of 32. It will be possible to re-enable it once const generics are stabilized. For now they're implemented manually.cargo benchresults are unaffected - or might be within experimental noise of 2% or so, hard to tell without statistical analysis on benchmark results.