-
Notifications
You must be signed in to change notification settings - Fork 37
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
Reduce unsafe code #131
Comments
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
There are mutliple uses of unsafe that can be either replaced with safe code or with an external crate. I propose using this issue to discuss these cases and then document in the code why each unsafe is fine. This makes code review much easier.
match
statements.Arc
without weak counter fromservo_arc
. The discussion to include this instd
died here. Why not use the crate? The currently published version has possible UB Currently-published servo_arc contains undefined behavior servo/servo#26358TranspositionTable
that allows to trigger undefined behaviour from safe code (if I understand the code correctly):I did not look into the usecases yet. There are probably alternatives available in the ecosystem.
TimeManager
: Internals can be replaced with atomics andOrdering::Relaxed
load/store (although theInstance
might be problematic). Also: do not create mutable references fromUnsafeCell
in unguarded code but use ptr::write to avoid UB.I am quite sure that the compiler will elide the bound checks. Another possibility is to use enum_map or static asserts.
static mut
and replace it either with lazy_static or with const fn initialization. https://github.com/sfleischman105/Pleco/blob/292d38e78dd7d82112aef79a379e8329707e3659/pleco_engine/src/tables/pawn_table.rs#L82MoveList
.mem::uninitialized
to gether with an enum that encodes if a field is used or not. Is there a reason that this is arepr(u8)
enum instead of carrying the data itself? https://github.com/sfleischman105/Pleco/blob/292d38e78dd7d82112aef79a379e8329707e3659/pleco_engine/src/movepick/mod.rs#L64The text was updated successfully, but these errors were encountered: