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 RefCounted.unreference() documentation providing wrong info. #82557

Merged
merged 1 commit into from
Oct 11, 2023
Merged

Fix RefCounted.unreference() documentation providing wrong info. #82557

merged 1 commit into from
Oct 11, 2023

Conversation

RadiantUwU
Copy link
Contributor

No description provided.

@akien-mga akien-mga changed the title Fix RefCounted.unreference() providing wrong info. Fix RefCounted.unreference() documentation providing wrong info. Sep 30, 2023
@akien-mga akien-mga added the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Sep 30, 2023
@RandomShaper
Copy link
Member

The docs are wrong, but the suggested description is not accurate either RefCounted::unference() won't kill/free/release the object. Simply put, it decreases the reference count and returns true if it's 0 after the decrement. That can happen either because the call made it go from 1 to 0, or because buggy code called it again when it was already 0. The buggy caller case aside, true means that the object should be freed since no-one is referencing it.

@RadiantUwU
Copy link
Contributor Author

The docs are wrong, but the suggested description is not accurate either RefCounted::unference() won't kill/free/release the object. Simply put, it decreases the reference count and returns true if it's 0 after the decrement. That can happen either because the call made it go from 1 to 0, or because buggy code called it again when it was already 0. The buggy caller case aside, true means that the object should be freed since no-one is referencing it.

Yeah but then it notifies the script of NOTIFICAITON_PREDELETE and if it runs .cancel_free() its gonna set die to false which is then returned as false, resulting in the object not being freed.

@RandomShaper
Copy link
Member

Either you or me are missing something. Can you please show your point with a piece of GDScript or C++? Code will act as a universal language.

@RadiantUwU
Copy link
Contributor Author

RadiantUwU commented Oct 4, 2023

The docs are wrong, but the suggested description is not accurate either RefCounted::unference() won't kill/free/release the object. Simply put, it decreases the reference count and returns true if it's 0 after the decrement. That can happen either because the call made it go from 1 to 0, or because buggy code called it again when it was already 0. The buggy caller case aside, true means that the object should be freed since no-one is referencing it.

I tested it with the code below

extends RefCounted
class_name TestRefCounted

func _notification(what):
	if self == null: return
	if what == NOTIFICATION_PREDELETE:
		print("Attempt to delete refcounted!")
		cancel_free()
extends Node

func _ready():
	var ref:=TestRefCounted.new()
	var weakref:WeakRef=weakref(ref)
	print(ref.get_reference_count())
	ref = null
	ref = weakref.get_ref()
	print(ref)

This doesnt even send a notification to delete it...
This is real strange.

@RandomShaper
Copy link
Member

Thanks for the sample!

A couple of thoughts:

  • Regarding the missing notification, and probably why you are null-checking, it's a known "issue". See Calling a method of your own Reference script within PREDELETE notification fails #31166. I'd say it's by-design. Preventing a Reference from freeing its referred object would be fighting against the engine.
  • Regarding the main point here, note that you are not calling unreference() directly. GDScript variables are Variant under the hood. Now, Variant, like Reference, manages the refcount of a RefCounted object it is referencing. So, when you do ref = null, you are letting Variant call unreference() on the RefCounted. And if such call returns true, it frees the object.

@RadiantUwU
Copy link
Contributor Author

Updated it. is it fine now?

@RandomShaper
Copy link
Member

The only pending thing would be squashing the four commits into one.

See https://docs.godotengine.org/en/stable/contributing/workflow/pr_workflow.html#the-interactive-rebase.

@RadiantUwU
Copy link
Contributor Author

The only pending thing would be squashing the four commits into one.

See https://docs.godotengine.org/en/stable/contributing/workflow/pr_workflow.html#the-interactive-rebase.

... i thought that github does that automatically no?

@akien-mga
Copy link
Member

Only if using the "Squash and Merge" feature, but we don't use it for a number of reasons. We prefer that PRs get squashed before merge.

@RadiantUwU
Copy link
Contributor Author

Done

Copy link
Member

@RandomShaper RandomShaper left a comment

Choose a reason for hiding this comment

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

Thanks for handling the feedback with patience.

@akien-mga
Copy link
Member

For the record, your commit seems not to be linked to your GitHub account. See: Why are my commits linked to the wrong user? for more info.

@RadiantUwU
Copy link
Contributor Author

For the record, your commit seems not to be linked to your GitHub account. See: Why are my commits linked to the wrong user? for more info.

Yeah i know about that issue thanks!

@akien-mga akien-mga merged commit 357a57b into godotengine:master Oct 11, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Oct 24, 2023
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.3.

@RadiantUwU RadiantUwU deleted the patch-2 branch October 25, 2023 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants