Remove static mut in graphics_device.rs
#674
Merged
Conversation
This file contains 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
lionel-
force-pushed
the
task/gd-static-mut
branch
from
26ce4d6 to
e76f980
Compare
lionel-
force-pushed
the
task/gd-static-mut
branch
from
e76f980 to
5fa82bf
Compare
DavisVaughan
approved these changes
Jan 24, 2025
| comm_manager_tx: Sender<CommManagerEvent>, | ||
| iopub_tx: Sender<IOPubMessage>, | ||
| dynamic_plots: bool, | ||
| ) { | ||
| let id = unwrap!(self._id.clone(), None => { | ||
| // Refcell Safety: Short borrows in the file. |
There was a problem hiding this comment.
Is the comment wrong? More like Cloning to borrow for as short as possible
There was a problem hiding this comment.
It's referring to other borrows made by the device context. Not sure how best to phrase this.
| @@ -271,10 +279,13 @@ impl DeviceContext { | |||
| } | |||
|
|
|||
| // Save our new socket. | |||
| self._channels.insert(id.to_string(), socket.clone()); | |||
| // Refcell Safety: Short borrows in the file. | |||
There was a problem hiding this comment.
What does "in the file" mean?
There was a problem hiding this comment.
DeviceContext is private to the file.
lionel-
force-pushed
the
task/gd-static-mut
branch
from
6d2a559 to
49d2cfa
Compare
|
Logging this from Slack for reference: Atomics are for multi-threaded accesses but here it's single-threaded so using Since this is all single-threaded, using |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Progress towards #661.
DEVICE_CONTEXTis now a thread-local variable to reflect the fact it should only be accessed from the R thread.It's wrapped in a
RefCellthat is initialized fromRMainon startup. All subsequent accesses are read-only and never panic.The device context requires mutable state. Where possible we use
Cellwhich never panics. In a couple of places we useRefCellwith care not to double-borrow on mutation, mainly by only performing short borrows to prevent holding a reference while recusing into a device context method by accident. (I haven't examined whether that would be possible in principle to recurse into a method as I'm not familiar with the graphics device code and its R hooks. I just made sure the borrows were short or at least only invoked pure Rust code.)