Make PhysicalMapping safely Send / Sync - #361
Open
martin-hughes wants to merge 2 commits into
Open
martin-hughes wants to merge 2 commits into
martin-hughes wants to merge 2 commits into
Conversation
This is a prerequisite to removing the `pub` modifiers. Without moving these structs, the fields are visible to the whole crate, which removes much of the benefit of making the interface safer.
Contributor
Author
|
Definitely should have tested this after rebasing onto the newest |
The internals of `RawPhysicalMapping` and `PhysicalMapping` have been hidden, and can now only ba accessed via accessors. This is largely for the purpose of hiding `virtual_start`, to allow for an interface that can be made Send & Sync. The other fields are hidden for consistency.
martin-hughes
force-pushed
the
safer-physical-mapping
branch
from
September 23, 2026 19:50
d9ce717 to
fc32783
Compare
martin-hughes
changed the title
Contributor
Author
|
I've had a think about it, and decided that the Ready for review. |
martin-hughes
marked this pull request as ready for review
September 23, 2026 19:52
martin-hughes
commented
Sep 23, 2026
|
|
||
| // If all other types are correctly labelled with Send and/or Sync, then Interpreter should | ||
| // naturally become Send/Sync. | ||
| assert_impl_all!(Interpreter<NullHandler>: Send, Sync); |
Contributor
Author
There was a problem hiding this comment.
Probably superfluous given that we've added unsafe impls, but I've left it in as a statement of intent / so it won't be forgotten.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The aim of this PR is to close #324 by getting the correct Send/Sync annotations on relevant types (such as
PhysicalMappingandWrappedObject).This is a work-in-progress. The outstanding work is to figure out what to do with
NativeMethod.As mentioned in #324,
PhysicalMappingpresented an interface that was not truly Send or Sync due to the public access to thevirtual_start: NonNull<T>.I've made it correct by hiding that field (* although access is still allowed if needed through an
unsafefunction!) / only allowing access through the deref methods, and I've increased the difficulty of misusingPhysicalMappingandRawPhysicalMappingby making their fields be private.That would fix @ChocolateLoverRaj's initial request in #324 to have
SyncforFixedRegisters.If
WrappedObjectisSend + Syncthen this would allowInterpreterto truly beSend + Sync, thus fixing @ChocolateLoverRaj's request to be able to store Interpreter globally. However, viaObjectandNativeMethod,WrappedObjectis notSend + Sync.In this PR I've marked it as though it is - it tries to be, but isn't. For this PR to be ready, those lines really need removing.
Happy to take thoughts/comments from anyone interested: @ChocolateLoverRaj, @IsaacWoods and anyone else.