0.15.1 (older version) Thoroughness: Medium Understanding: Medium
by MaulingMonkey on 2019-09-19
This review is from Crev, a distributed system for code reviews. To add your review, set up cargo-crev.
The current version of Specs is 0.20.0.
Lib.rs has been able to verify that all files in the crate's tarball, except Cargo.lock,
are in the crate's repository. Please note that this check is still in beta, and absence of this confirmation does not mean that the files don't match.
Crates in the crates.io registry are tarball snapshots uploaded by crates' publishers. The registry is not using crates' git repositories, so there is a possibility that published crates have a misleading repository URL, or contain different code from the code in the repository.
To review the actual code of the crate, it's best to use cargo crev open specs. Alternatively, you can download the tarball of specs v0.20.0 or view the source online.
Pros:
Cons:
println!()s for !is_unconstrained() joins is lame / a perf hazard in and of itself.-1s for more details0.15.1
Misc. notes:
world.maintain()is necessaryrust,ignores, poor link namesdispatcher.with_barrier()Entries::getseems sketchy&mut Storage::get_mutis sketchy as heckParJoinI can't wrap my head around,_uncheckedfns sound unsafe but aren't marked as such.UnprotectedStorageis a scary API in general for perf reasons.self.killed.contains(...)checks inAllocator, ungrokkedParJoincreate_entity_unchecked,is_alivesrc/join/mod.rs
src/join/par_join.rs
unsafe { ... }relies on constraint of implsrc/storage/drain.rs
src/storage/entry.rs
unsafe { ... }documents and abides by safety constraintsunsafe { ... }- I can't verify how this lifetime extension is possibly sound either.unsafe { ... }- documents and abides by safety constraintsunsafe { ... }- documents and abides by safety constraintsunsafe { ... }- documents and abides by safety constraintsunsafe { ... }- documents and abides by safety constraintssrc/storage/flagged.rs
src/storage/mod.rs
unsafe { ... }- documents and abides by safety constraintsunsafe { ... }- documents and abides by safety constraintsunsafe { ... }- documents and abides by safety constraintsunsafe { ... }- documents and abides by safety constraintsunsafe { ... }- documents and abides by safety constraintsunsafe { ... }- documents and abides by safety constraintsunsafe { ... }- documents and abides by safety constraintsclearmust be called before dropping as currently stands, unless that's just a bug.src/storage/restrict.rs
unsafe { ... }- maybe fine, but safety undocumented and get_unchecked sounds like something that should itself be an unsafe fn?unsafe { ... }- maybe fine, but safety undocumented and get_unchecked sounds like something that should itself be an unsafe fn?src/storage/storages.rs
Careful auditing gives a good idea for what the actual API requirements of
UnprotectedStorageare, and it's a doozy.cleancleans on several storages make me think it should consumeselfinstead of taking&mut self- maybe can't due to API limitations?set_lenleaves uninitialized gaps in the data_id Vec. Maybe sound, but at least violates std docs. See #644Default::default()set_lenleaves uninitialized gaps in the data_id Vec, making it unsound to drop before calling clean. Sound if perfectly used (e.g. call clean), but a timebomb as stands IMO. See #644src/storage/tests.rs
unsafe { ... }src/storage/track.rs
unsafe { ... }- I don't grok the safety invariants we need to hold hereunsafe { ... }- I don't grok the safety invariants we need to hold hereunsafe { ... }- I don't grok the safety invariants we need to hold hereunsafe { ... }- I don't grok the safety invariants we need to hold heresrc/world/entity.rs
Well, this is a bit more complicated than simply doing generational indicies. Atomics... oh boy.
generations, used forJoinand not much else. Lags behind when *_atomic is used.src/world/world_ext.rs
unsafe fn? What isn't checked? Just that you have exclusive world access?src/bitset.rs
src/changeset.rs
unsafe { ... }- documents and abides by safety constraintsunsafe { ... }- abides by safety constraints, docs seem a little offunsafe { ... }- documents and abides by safety constraintsTIL
#[must_use]makes a lot of sense for builder patterns terminating in.build()