Merge pull request #1973 from dsnopek/memnew-refcounted

Make `memnew(RefCounted)` return `Ref<T>` for improved ownership safety

Author: dsnopek

include/godot_cpp/classes/ref.hpp

@@ -191,7 +191,8 @@ class Ref {
 
 	template <typename... VarArgs>
 	void instantiate(VarArgs... p_params) {
-		ref(memnew(T(p_params...)));
+		Ref<T> ref = memnew(T(p_params...));
+		SWAP(reference, ref.reference);
 	}
 
 	uint32_t hash() const { return HashMapHasherDefault::hash(reference); }
@@ -211,6 +212,25 @@ class Ref {
 	}
 };
 
+template <typename T>
+struct memnew_result<T, std::enable_if_t<std::is_base_of_v<RefCounted, T>>> {
+#if GODOT_VERSION_MINOR >= 7
+	using class_name = Ref<T>;
+	_ALWAYS_INLINE_ static class_name capture(T *p_obj) {
+		// Godot will have already incremented the refcount, so we can create the Ref<T> without incrementing it again.
+		return Ref<T>::_gde_internal_constructor(p_obj);
+	}
+#else
+	using class_name = T *;
+	_ALWAYS_INLINE_ static class_name capture(class_name p_obj) { return p_obj; }
+#endif
+};
+
+template <typename T>
+void postinitialize_handler(Ref<T> &p_object) {
+	postinitialize_handler(p_object.ptr());
+}
+
 template <typename T>
 struct PtrToArg<Ref<T>> {
 	_FORCE_INLINE_ static Ref<T> convert(const void *p_ptr) {

include/godot_cpp/core/class_db.hpp

@@ -255,7 +255,9 @@ void ClassDB::_register_class(bool p_virtual, bool p_exposed, bool p_runtime) {
 	class_register_order.push_back(cl.name);
 
 	// Register this class with Godot
-#if GODOT_VERSION_MINOR >= 5
+#if GODOT_VERSION_MINOR >= 7
+	GDExtensionClassCreationInfo6 class_info = {
+#elif GODOT_VERSION_MINOR >= 5
 	GDExtensionClassCreationInfo5 class_info = {
 #elif GODOT_VERSION_MINOR >= 4
 	GDExtensionClassCreationInfo4 class_info = {
@@ -292,7 +294,9 @@ void ClassDB::_register_class(bool p_virtual, bool p_exposed, bool p_runtime) {
 		(void *)&T::get_class_static(), // void *class_userdata;
 	};
 
-#if GODOT_VERSION_MINOR >= 5
+#if GODOT_VERSION_MINOR >= 7
+	::godot::gdextension_interface::classdb_register_extension_class6(::godot::gdextension_interface::library, cl.name._native_ptr(), cl.parent_name._native_ptr(), &class_info);
+#elif GODOT_VERSION_MINOR >= 5
 	::godot::gdextension_interface::classdb_register_extension_class5(::godot::gdextension_interface::library, cl.name._native_ptr(), cl.parent_name._native_ptr(), &class_info);
 #elif GODOT_VERSION_MINOR >= 4
 	::godot::gdextension_interface::classdb_register_extension_class4(::godot::gdextension_interface::library, cl.name._native_ptr(), cl.parent_name._native_ptr(), &class_info);

include/godot_cpp/core/memory.hpp

@@ -84,12 +84,22 @@ class Memory {
 template <typename T, std::enable_if_t<!std::is_base_of<::godot::Wrapped, T>::value, bool> = true>
 _ALWAYS_INLINE_ void _pre_initialize() {}
 
+template <typename T, typename Enable = void>
+struct memnew_result {
+	using class_name = T *;
+	_ALWAYS_INLINE_ static class_name capture(T *p_obj) { return p_obj; }
+};
+
+template <typename T>
+using memnew_result_t = typename memnew_result<T>::class_name;
+
 _ALWAYS_INLINE_ void postinitialize_handler(void *) {}
 
 template <typename T>
-_ALWAYS_INLINE_ T *_post_initialize(T *p_obj) {
+_ALWAYS_INLINE_ memnew_result_t<T> _post_initialize(T *p_obj) {
+	memnew_result_t<T> result(memnew_result<T>::capture(p_obj));
 	postinitialize_handler(p_obj);
-	return p_obj;
+	return result;
 }
 
 #define memalloc(m_size) ::godot::Memory::alloc_static(m_size)

src/classes/wrapped.cpp

@@ -81,7 +81,9 @@ Wrapped::Wrapped(const StringName &p_godot_class) {
 	} else
 #endif
 	{
-#if GODOT_VERSION_MINOR >= 4
+#if GODOT_VERSION_MINOR >= 7
+		_owner = ::godot::gdextension_interface::classdb_construct_object3(reinterpret_cast<GDExtensionConstStringNamePtr>(p_godot_class._native_ptr()));
+#elif GODOT_VERSION_MINOR >= 4
 		_owner = ::godot::gdextension_interface::classdb_construct_object2(reinterpret_cast<GDExtensionConstStringNamePtr>(p_godot_class._native_ptr()));
 #else
 		_owner = ::godot::gdextension_interface::classdb_construct_object(reinterpret_cast<GDExtensionConstStringNamePtr>(p_godot_class._native_ptr()));

test/project/main.gd

@@ -44,6 +44,7 @@ func _ready():
 	# Pass custom reference.
 	assert_equal(example.custom_ref_func(null), -1)
 	var ref1 = ExampleRef.new()
+	assert_equal(ref1.get_reference_count(), 1)
 	ref1.id = 27
 	assert_equal(example.custom_ref_func(ref1), 27)
 	ref1.id += 1;
@@ -63,6 +64,7 @@ func _ready():
 	assert_equal(null_ref, null)
 	var ret_ref = example.return_extended_ref()
 	assert_not_equal(ret_ref.get_instance_id(), 0)
+	assert_equal(ret_ref.get_reference_count(), 1)
 	assert_equal(ret_ref.get_id(), 0)
 	assert_equal(example.get_v4(), Vector4(1.2, 3.4, 5.6, 7.8))
 	assert_equal(example.test_node_argument(example), example)

test/src/example.cpp

@@ -379,10 +379,7 @@ Ref<ExampleRef> Example::return_empty_ref() const {
 	return ref;
 }
 
-ExampleRef *Example::return_extended_ref() const {
-	// You can instance and return a refcounted object like this, but keep in mind that refcounting starts with the returned object
-	// and it will be destroyed when all references are destroyed. If you store this pointer you run the risk of having a pointer
-	// to a destroyed object.
+Ref<ExampleRef> Example::return_extended_ref() const {
 	return memnew(ExampleRef());
 }
 

test/src/example.h

@@ -124,7 +124,7 @@ class Example : public Control {
 	Viewport *return_something_const() const;
 	Ref<ExampleRef> return_ref() const;
 	Ref<ExampleRef> return_empty_ref() const;
-	ExampleRef *return_extended_ref() const;
+	Ref<ExampleRef> return_extended_ref() const;
 	Ref<ExampleRef> extended_ref_checks(Ref<ExampleRef> p_ref) const;
 	Variant varargs_func(const Variant **args, GDExtensionInt arg_count, GDExtensionCallError &error);
 	int varargs_func_nv(const Variant **args, GDExtensionInt arg_count, GDExtensionCallError &error);