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

Improve API soundness wrt reference counted Gd #377

Merged
merged 1 commit into from
Aug 28, 2023

Conversation

lilizoey
Copy link
Member

@lilizoey lilizoey commented Aug 9, 2023

Document the safety invariants that need to be upheld for reference counted Gd.
Make Mem::maybe_dec_ref an unsafe function, as it can be used to bypass the safety invariants of a reference counted Gd.

The maybe_dec_ref api is admittedly private (doc-hidden), however i still believe it is useful for us to properly label it as unsafe anyway. So we can more easily tell that it's being used correctly.

@lilizoey
Copy link
Member Author

lilizoey commented Aug 9, 2023

I didn't include making Domain::scoped_mut unsafe here (as was done in #349). Since thinking about it, it's only unsafe because we don't perform validity checks in methods that expect the object to be valid. And that's something we're planning on fixing.

@GodotRust
Copy link

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-377

@lilizoey
Copy link
Member Author

lilizoey commented Aug 9, 2023

I just realized that i should maybe see if i can make RefCounted::unreference unsafe too

@Bromeon
Copy link
Member

Bromeon commented Aug 9, 2023

The RefCounted methods to mess with the ref-counter have been deliberately removed from the public API:
https://godot-rust.github.io/docs/gdext/master/godot/engine/struct.RefCounted.html

As such I don't think this note is really needed. Not messing around in GDScript is already covered in the safety model (which I finally need to upload...)

Making maybe_dec_ref() unsafe is a good idea though.

@Bromeon
Copy link
Member

Bromeon commented Aug 27, 2023

@lilizoey if you could remove the safety docs for Gd itself, this should be ready. The docs + unsafe in Memory are good as-is.

Make `Mem::maybe_dec_ref` an unsafe function
@lilizoey lilizoey force-pushed the fix/unsafe-memory-functions branch from fc66f73 to e031e8e Compare August 28, 2023 08:13
@Bromeon Bromeon enabled auto-merge August 28, 2023 08:16
@Bromeon Bromeon added bug c: core Core components ub Undefined behavior labels Aug 28, 2023
@Bromeon Bromeon added this pull request to the merge queue Aug 28, 2023
Merged via the queue into godot-rust:master with commit 2b6fce2 Aug 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug c: core Core components ub Undefined behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants