-
Notifications
You must be signed in to change notification settings - Fork 113
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
Fix wrong pointer type of static _dispatch_main_q
binding
#343
Fix wrong pointer type of static _dispatch_main_q
binding
#343
Conversation
CI correctly suggests usage of the new |
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 it might technically be a soundness fix, since we now go from telling the compiler that _dispatch_main_q
has the size of a pointer (and asserting the validity of that by creating a reference in &_dispatch_main_q
), to using Object
which is zero-sized, and hence acts as-if the object may have an arbitrary size (in a similar way that extern types do).
But I doubt it matters at all in practice (esp. since LLVM migrated to opaque pointers).
Use of
mut
here is debatable.
Yeah, if we want to be really correct, I'd recommend something like _dispatch_main_q: SyncUnsafeCell<Object>
.
MSRV
You can use std::ptr::addr_of[_mut]!
instead (and #[allow(...)]
the clippy warning for that).
53270f3
to
780f9b5
Compare
While looking at a copy of this code (in another project that uses `objc2`, which does not yet define `dispatch` integration) this strange cast immediately stands out. `_dispatch_main_q` is defined here as a `dispatch_queue_t`, the `queue` argument in `dispatch_data_create()` is also defined as a `dispatch_queue_t`, yet there is a "borrow" (get *pointer to*) operation before passing it into the function. Instead, this `static` field is supposed to be defined as the `Object` itself, so that any user could take a _pointer to it_ if they wish or need it so. The same is seen in [the `dispatch` crate]: `_dispatch_main_q` is defined as a `dispatch_object_s`, and `dispatch_queue_t` is a _typedef_ to a `*mut dispatch_object_s` to match the above convention. Note that this is not a bugfix, but merely a readability improvement. Use of `mut` here is debatable. [the `dispatch` crate]: https://github.com/SSheldon/rust-dispatch/blob/f540a2d8ccaebf0e87f5805033b9e287e8d01ba5/src/ffi.rs#L33
780f9b5
to
ac36047
Compare
@cwfitzgerald: Now that Firefox has an MSRV of Rust 1.82, I wouldn't object to a bump to Rust 1.82 (as follow-up) so we can use |
Yeah that's fine. |
While looking at a copy of this code (in another project that uses
objc2
, which does not yet definedispatch
integration) this strange cast immediately stands out._dispatch_main_q
is defined here as adispatch_queue_t
, thequeue
argument indispatch_data_create()
is also defined as adispatch_queue_t
, yet there is a "borrow" (get pointer to) operation before passing it into the function.Instead, this
static
field is supposed to be defined as theObject
itself, so that any user could take a pointer to it if they wish or need it so. The same is seen in thedispatch
crate:_dispatch_main_q
is defined as adispatch_object_s
, anddispatch_queue_t
is a typedef to a*mut dispatch_object_s
to match the above convention.Note that this is not a bugfix, but merely a readability improvement. Use of
mut
here is debatable.