Skip to content
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 static mut issues with routines #677

Open
wants to merge 2 commits into
base: task/gd-static-mut
Choose a base branch
from

Conversation

lionel-
Copy link
Contributor

@lionel- lionel- commented Jan 24, 2025

Closes #661.

Ideally R_ROUTINES would be thread-local but that's currently inconvenient in unit tests.

@lionel- lionel- requested a review from DavisVaughan January 24, 2025 14:54
Comment on lines +50 to +56
// Now unwrap the definitions from our thread-safe type
let unwrapped: Vec<R_CallMethodDef> = R_ROUTINES
.lock()
.unwrap()
.iter()
.map(|def| def.inner)
.collect();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fairly surprised you dont get a warning about "dropping temporary value" since no one is holding the lock in a local variable here. I seem to get that all the time when doing stuff like this.

name: std::ptr::null(),
fun: None,
numArgs: 0,
});

// Now unwrap the definitions from our thread-safe type
let unwrapped: Vec<R_CallMethodDef> = R_ROUTINES
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let unwrapped: Vec<R_CallMethodDef> = R_ROUTINES
let routines: Vec<R_CallMethodDef> = R_ROUTINES

R_registerRoutines(
info,
std::ptr::null(),
R_ROUTINES.as_ptr(),
unwrapped.as_ptr(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
unwrapped.as_ptr(),
routines.as_ptr(),

unsafe impl Send for CallMethodDef {}
unsafe impl Sync for CallMethodDef {}

static R_ROUTINES: Mutex<Vec<CallMethodDef>> = Mutex::new(vec![]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice I also tried this when fiddling with this one last week but got stuck on the R_CallMethodDef not being send+sync problem

Comment on lines +21 to +22
unsafe impl Send for CallMethodDef {}
unsafe impl Sync for CallMethodDef {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think my main comment is that #[harp::register] is pretty freaking magical. It generates some code that runs at application startup time in an OS specific way.

I guess we can assume that code is always run on a single thread? I have no idea how startup code gets run on a per OS basis. I just want to make sure this doesn't screw us up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants