Conversation
There was a problem hiding this comment.
Pull request overview
Updates the Rust-side # Safety documentation around an unsafe call in the evdev-rs input event processing path, aiming to better justify the use of pin_mut_unchecked() when removing devices from the C++ InputDeviceRegistry.
Changes:
- Replaced a generic safety note with a more specific explanation referencing
cxx-rsandSharedPtr<Opaque>. - Clarified the intended safety rationale for calling
remove_device()throughpin_mut_unchecked().
| // InputDeviceRegistry is an opaque C++ type; cxx-rs guarantees that calling | ||
| // pin_mut_unchecked on a SharedPtr<Opaque> is safe for calling thread-safe | ||
| // non-const member functions (which `remove_device` is). |
There was a problem hiding this comment.
The updated # Safety justification is likely inaccurate/misleading: cxx::SharedPtr::pin_mut_unchecked() is unsafe because Rust cannot prove unique/exclusive mutable access from a shared_ptr (and cannot validate thread-safety or const-correctness of the underlying C++ method). Rather than saying “cxx-rs guarantees … is safe”, document the actual invariants you are asserting here (e.g., no other Rust/C++ aliases are used concurrently for this object, and the C++ type internally synchronizes any shared mutation if it can be accessed from multiple threads).
There was a problem hiding this comment.
I tend to agree with copilot here: it is the underlying C++ (shared_ptr<>) that provides any guarantee, not pin_mut_unchecked() itself.
There was a problem hiding this comment.
Copilot is incorrect here; cxx-rs provides the guarantee that cxx::SharedPtr::pin_mut_unchecked() is sound for calling (thread-safe) methods on opaque C++ types regardless of whether other references exist.
std::Pin has significant invariants that are not provided by std::shared_ptr.
Specifically, the invariant that I'm asserting here is that InputDeviceRegistry is an opaque C++ type (which is the invariant that cxx-rs requires for its invariants to be upheld).
There was a problem hiding this comment.
OK, I RTFM'd: "For the special case of CXX opaque C++ types, this method can be used to safely call thread-safe non-const member functions on a C++ object without regard for whether the reference is exclusive."
That wasn't my initial reading of the proposed comment, but I now see what you mean.
| // InputDeviceRegistry is an opaque C++ type; cxx-rs guarantees that calling | ||
| // pin_mut_unchecked on a SharedPtr<Opaque> is safe for calling thread-safe | ||
| // non-const member functions (which `remove_device` is). |
There was a problem hiding this comment.
OK, I RTFM'd: "For the special case of CXX opaque C++ types, this method can be used to safely call thread-safe non-const member functions on a C++ object without regard for whether the reference is exclusive."
That wasn't my initial reading of the proposed comment, but I now see what you mean.
github-merge-queue
bot
removed this pull request from the merge queue due to a conflict with the base branch
TICS Quality Gate✔️ Passedmir
|
No description provided.