From 6f58d752b7267e43bf709329d8b8f240e422ed66 Mon Sep 17 00:00:00 2001 From: Chitose Yuuzaki Date: Wed, 22 Feb 2023 11:10:17 +0000 Subject: [PATCH] Unify string parameters across the repo The types of string parameters across the repo has been regularized according to the following rules that shall be followed from now on: - Methods during the init/registration process should take `&str` for things like identifiers. They may specify explicit lifetimes in their types if necessary, but *not* `'static`. - Manually written methods that abstract over, or mimic API methods should take `impl Into` or `impl Into` depending on the usage, to be consistent with their generated counterparts. Regarding the usage of `&str` in init instead of more specific conversions required by each method: The GDNative API isn't very consistent when it comes to the string types it wants during init. Sometimes, functions may want different types of strings even when they are concepturally similar, like for signal and method registration. This gets more complicated when we add our own library-side logic into the mix, like for the `InitHandle::add_class` family, where allocation is current inevitable even when they are given `&'static str`s. It still makes sense to avoid excessive allocation, but that should not get in the way of future development. `'static` in particular is extremely limiting, and there are very few cases of its usage throughout the library's history that we have not since regretted. It shouldn't be the norm but the rare exception. In any case, class registration is something that only run once in the lifecycle of a game, and for the amount of classes most people are using with `gdnative`, we can afford to be slightly inefficient with strings to make our APIs more consistent, less leaky, and easier to stablize and maintain. --- gdnative-async/src/rt.rs | 10 ++++----- gdnative-async/src/rt/bridge.rs | 4 ++-- gdnative-bindings/src/utils.rs | 2 +- gdnative-core/src/export/class_builder.rs | 6 ------ gdnative-core/src/export/class_registry.rs | 18 +++++++++------- .../src/export/property/invalid_accessor.rs | 4 ++-- gdnative-core/src/init/init_handle.rs | 21 +++++++++---------- gdnative-core/src/object/instance.rs | 3 ++- gdnative/src/globalscope.rs | 4 ++-- 9 files changed, 34 insertions(+), 38 deletions(-) diff --git a/gdnative-async/src/rt.rs b/gdnative-async/src/rt.rs index da736d6ea..d7d49862f 100644 --- a/gdnative-async/src/rt.rs +++ b/gdnative-async/src/rt.rs @@ -3,7 +3,7 @@ use std::marker::PhantomData; use func_state::FuncState; use gdnative_bindings::Object; -use gdnative_core::core_types::{GodotError, Variant}; +use gdnative_core::core_types::{GodotError, GodotString, Variant}; use gdnative_core::init::InitHandle; use gdnative_core::object::{Instance, SubClass, TInstance, TRef}; @@ -77,13 +77,13 @@ impl Context { pub fn signal( &self, obj: TRef<'_, C>, - signal: &str, + signal: impl Into, ) -> Result>, GodotError> where C: SubClass, { let (future, resume) = future::make(); - bridge::SignalBridge::connect(obj.upcast(), signal, resume)?; + bridge::SignalBridge::connect(obj.upcast(), signal.into(), resume)?; Ok(future) } } @@ -107,8 +107,8 @@ pub fn register_runtime_with_prefix(handle: &InitHandle, prefix: S) where S: Display, { - handle.add_class_as::(format!("{prefix}SignalBridge")); - handle.add_class_as::(format!("{prefix}FuncState")); + handle.add_class_as::(&format!("{prefix}SignalBridge")); + handle.add_class_as::(&format!("{prefix}FuncState")); } /// Releases all observers still in use. This should be called in the diff --git a/gdnative-async/src/rt/bridge.rs b/gdnative-async/src/rt/bridge.rs index a8c2b8d26..2721237b1 100644 --- a/gdnative-async/src/rt/bridge.rs +++ b/gdnative-async/src/rt/bridge.rs @@ -4,7 +4,7 @@ use once_cell::sync::OnceCell; use parking_lot::Mutex; use gdnative_bindings::{Object, Reference}; -use gdnative_core::core_types::{GodotError, Variant, VariantArray}; +use gdnative_core::core_types::{GodotError, GodotString, Variant, VariantArray}; use gdnative_core::export::user_data::{ArcData, Map}; use gdnative_core::export::{ClassBuilder, Method, NativeClass, NativeClassMethods, Varargs}; use gdnative_core::godot_site; @@ -58,7 +58,7 @@ impl NativeClass for SignalBridge { impl SignalBridge { pub(crate) fn connect( source: TRef, - signal: &str, + signal: GodotString, resume: Resume>, ) -> Result<(), GodotError> { let mut pool = BRIDGES.get_or_init(Mutex::default).lock(); diff --git a/gdnative-bindings/src/utils.rs b/gdnative-bindings/src/utils.rs index 5e49b2e91..65fda9bbb 100644 --- a/gdnative-bindings/src/utils.rs +++ b/gdnative-bindings/src/utils.rs @@ -18,7 +18,7 @@ use super::generated::{Engine, Node, SceneTree}; /// invariants must be observed for the resulting node during `'a`, if any. /// /// [thread-safety]: https://docs.godotengine.org/en/stable/tutorials/threads/thread_safe_apis.html -pub unsafe fn autoload<'a, T>(name: &str) -> Option> +pub unsafe fn autoload<'a, T>(name: impl Into) -> Option> where T: SubClass, { diff --git a/gdnative-core/src/export/class_builder.rs b/gdnative-core/src/export/class_builder.rs index eb5359499..60b64f274 100644 --- a/gdnative-core/src/export/class_builder.rs +++ b/gdnative-core/src/export/class_builder.rs @@ -10,12 +10,6 @@ use crate::export::*; use crate::object::NewRef; use crate::private::get_api; -// TODO(#996): unify string parameters across all buiders -// Potential candidates: -// * &str -// * impl Into -// * impl Into> - /// Allows registration of exported properties, methods and signals. /// /// See member functions of this class for usage examples. diff --git a/gdnative-core/src/export/class_registry.rs b/gdnative-core/src/export/class_registry.rs index 84171694b..e63a72819 100644 --- a/gdnative-core/src/export/class_registry.rs +++ b/gdnative-core/src/export/class_registry.rs @@ -1,5 +1,4 @@ use std::any::TypeId; -use std::borrow::Cow; use std::collections::hash_map::Entry; use std::collections::HashMap; use std::fmt; @@ -15,7 +14,7 @@ static CLASS_REGISTRY: Lazy>> = #[derive(Clone, Debug)] pub(crate) struct ClassInfo { - pub name: Cow<'static, str>, + pub name: String, pub init_level: InitLevel, } @@ -31,7 +30,7 @@ where /// Returns the NativeScript name of the class `C` if it is registered. /// Can also be used to validate whether or not `C` has been added using `InitHandle::add_class()` #[inline] -pub(crate) fn class_name() -> Option> { +pub(crate) fn class_name() -> Option { with_class_info::(|i| i.name.clone()) } @@ -41,8 +40,8 @@ pub(crate) fn class_name() -> Option> { /// The returned string should only be used for diagnostic purposes, not for types that the user /// has to name explicitly. The format is not guaranteed. #[inline] -pub(crate) fn class_name_or_default() -> Cow<'static, str> { - class_name::().unwrap_or_else(|| Cow::Borrowed(std::any::type_name::())) +pub(crate) fn class_name_or_default() -> String { + class_name::().unwrap_or_else(|| std::any::type_name::().into()) } /// Registers the class `C` in the class registry, using a custom name at the given level. @@ -51,14 +50,17 @@ pub(crate) fn class_name_or_default() -> Cow<'static, str> { /// Returns an error with the old `ClassInfo` if a conflicting entry for `C` was already added. #[inline] pub(crate) fn register_class_as( - name: Cow<'static, str>, + name: &str, init_level: InitLevel, ) -> Result { let type_id = TypeId::of::(); let mut registry = CLASS_REGISTRY.write(); match registry.entry(type_id) { Entry::Vacant(entry) => { - entry.insert(ClassInfo { name, init_level }); + entry.insert(ClassInfo { + name: name.into(), + init_level, + }); Ok(true) } Entry::Occupied(entry) => { @@ -86,7 +88,7 @@ pub(crate) fn register_class_as( #[inline] #[allow(dead_code)] // Currently unused on platforms with inventory support -pub(crate) fn types_with_init_level(allow: InitLevel, deny: InitLevel) -> Vec> { +pub(crate) fn types_with_init_level(allow: InitLevel, deny: InitLevel) -> Vec { let registry = CLASS_REGISTRY.read(); let mut list = registry .values() diff --git a/gdnative-core/src/export/property/invalid_accessor.rs b/gdnative-core/src/export/property/invalid_accessor.rs index 42861e065..7312daf58 100644 --- a/gdnative-core/src/export/property/invalid_accessor.rs +++ b/gdnative-core/src/export/property/invalid_accessor.rs @@ -84,7 +84,7 @@ unsafe impl<'l, C: NativeClass, T> RawSetter for InvalidSetter<'l> { let mut set = sys::godot_property_set_func::default(); let data = Box::new(InvalidAccessorData { - class_name: class_registry::class_name_or_default::().into_owned(), + class_name: class_registry::class_name_or_default::(), property_name: self.property_name.to_string(), }); @@ -101,7 +101,7 @@ unsafe impl<'l, C: NativeClass, T> RawGetter for InvalidGetter<'l> { let mut get = sys::godot_property_get_func::default(); let data = Box::new(InvalidAccessorData { - class_name: class_registry::class_name_or_default::().into_owned(), + class_name: class_registry::class_name_or_default::(), property_name: self.property_name.to_string(), }); diff --git a/gdnative-core/src/init/init_handle.rs b/gdnative-core/src/init/init_handle.rs index 78d43fa9e..681694f24 100644 --- a/gdnative-core/src/init/init_handle.rs +++ b/gdnative-core/src/init/init_handle.rs @@ -4,7 +4,6 @@ use crate::export::{ }; use crate::object::{GodotObject, RawObject, TRef}; use crate::private::get_api; -use std::borrow::Cow; use std::ffi::CString; use std::ptr; @@ -43,7 +42,7 @@ impl InitHandle { where C: NativeClassMethods + StaticallyNamed, { - self.add_maybe_tool_class_as_with::(Cow::Borrowed(C::CLASS_NAME), false, |builder| { + self.add_maybe_tool_class_as_with::(C::CLASS_NAME, false, |builder| { C::nativeclass_register_monomorphized(builder); f(builder); }) @@ -64,7 +63,7 @@ impl InitHandle { where C: NativeClassMethods + StaticallyNamed, { - self.add_maybe_tool_class_as_with::(Cow::Borrowed(C::CLASS_NAME), true, |builder| { + self.add_maybe_tool_class_as_with::(C::CLASS_NAME, true, |builder| { C::nativeclass_register_monomorphized(builder); f(builder); }) @@ -75,7 +74,7 @@ impl InitHandle { /// If the type implements [`StaticallyTyped`], that name is ignored in favor of the /// name provided at registration. #[inline] - pub fn add_class_as(self, name: String) + pub fn add_class_as(self, name: &str) where C: NativeClassMethods, { @@ -87,11 +86,11 @@ impl InitHandle { /// If the type implements [`StaticallyTyped`], that name is ignored in favor of the /// name provided at registration. #[inline] - pub fn add_class_as_with(self, name: String, f: impl FnOnce(&ClassBuilder)) + pub fn add_class_as_with(self, name: &str, f: impl FnOnce(&ClassBuilder)) where C: NativeClassMethods, { - self.add_maybe_tool_class_as_with::(Cow::Owned(name), false, f) + self.add_maybe_tool_class_as_with::(name, false, f) } /// Registers a new tool class to the engine @@ -99,7 +98,7 @@ impl InitHandle { /// If the type implements [`StaticallyTyped`], that name is ignored in favor of the /// name provided at registration. #[inline] - pub fn add_tool_class_as(self, name: String) + pub fn add_tool_class_as(self, name: &str) where C: NativeClassMethods, { @@ -111,23 +110,23 @@ impl InitHandle { /// If the type implements [`StaticallyTyped`], that name is ignored in favor of the /// name provided at registration. #[inline] - pub fn add_tool_class_as_with(self, name: String, f: impl FnOnce(&ClassBuilder)) + pub fn add_tool_class_as_with(self, name: &str, f: impl FnOnce(&ClassBuilder)) where C: NativeClassMethods, { - self.add_maybe_tool_class_as_with::(Cow::Owned(name), true, f) + self.add_maybe_tool_class_as_with::(name, true, f) } #[inline] fn add_maybe_tool_class_as_with( self, - name: Cow<'static, str>, + name: &str, is_tool: bool, f: impl FnOnce(&ClassBuilder), ) where C: NativeClassMethods, { - let c_class_name = CString::new(&*name).unwrap(); + let c_class_name = CString::new(name).unwrap(); match class_registry::register_class_as::(name, self.init_level) { Ok(true) => {} diff --git a/gdnative-core/src/object/instance.rs b/gdnative-core/src/object/instance.rs index 6951248bb..ae1dee9b5 100644 --- a/gdnative-core/src/object/instance.rs +++ b/gdnative-core/src/object/instance.rs @@ -1,3 +1,4 @@ +use std::borrow::Cow; use std::fmt::Debug; use std::ptr::NonNull; @@ -599,7 +600,7 @@ where fn from_variant(variant: &Variant) -> Result { let owner = Ref::::from_variant(variant)?; Self::from_base(owner).ok_or(FromVariantError::InvalidInstance { - expected: class_registry::class_name_or_default::(), + expected: Cow::Owned(class_registry::class_name_or_default::()), }) } } diff --git a/gdnative/src/globalscope.rs b/gdnative/src/globalscope.rs index 781ec7d29..8bc7b2d97 100644 --- a/gdnative/src/globalscope.rs +++ b/gdnative/src/globalscope.rs @@ -20,7 +20,7 @@ //! [@GDScript]: https://docs.godotengine.org/en/stable/classes/class_@gdscript.html use crate::api::{Resource, ResourceLoader}; -use crate::core_types::NodePath; +use crate::core_types::GodotString; use crate::object::{memory::RefCounted, GodotObject, Ref, SubClass}; #[doc(inline)] @@ -52,7 +52,7 @@ pub use gdnative_core::globalscope::*; /// let scene = load::("res://path/to/Main.tscn").unwrap(); /// ``` #[inline] -pub fn load(path: impl Into) -> Option> +pub fn load(path: impl Into) -> Option> where T: SubClass + GodotObject, {