Skip to content

Conversation

@lionel-
Copy link
Contributor

@lionel- lionel- commented Jun 23, 2023

This way all state needed by the callbacks can be inspected in one place and this makes the access syntax a bit lighter.
Also removes some unnecessary mutexes for state that is only accessed from frontend callbacks by the single R thread.

Branched from #51.

@lionel- lionel- force-pushed the refactor/read-console-state branch 2 times, most recently from 38b9121 to 45fca41 Compare June 23, 2023 18:06
@lionel- lionel- force-pushed the bugfix/prompt-type branch from c1888c3 to bf71e82 Compare June 23, 2023 18:06
@lionel-
Copy link
Contributor Author

lionel- commented Jun 26, 2023

Following this, we could move the frontend methods to impl RMain methods, here is an example: ffb6bbf

The idea is that we'd call let main = unsafe { R_MAIN.as_mut().unwrap() }; inside R extension points and then immediately the corresponding method on main. All these methods can then be written more conventionally with state in self, and without the R lock since the extension points are called synchronously by R (via frontend methods or .Call()). We just need to make sure the unsafe dereferences of the global R_MAIN object are indeed made from the R thread to ensure safety.

@lionel- lionel- requested a review from kevinushey June 26, 2023 19:35
@lionel- lionel- force-pushed the bugfix/prompt-type branch from bf71e82 to 91612f3 Compare June 26, 2023 20:48
@lionel- lionel- changed the base branch from bugfix/prompt-type to main June 26, 2023 20:56
Copy link
Contributor

@kevinushey kevinushey left a comment

Choose a reason for hiding this comment

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

LGTM!

@lionel- lionel- merged commit 0308851 into main Jun 28, 2023
@lionel- lionel- deleted the refactor/read-console-state branch June 28, 2023 07:28
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.

3 participants