Skip to content

Commit 63b942e

Browse files
beicauseBromeon
authored andcommitted
Use rust str instead of cstr for ClassIdSource
Replace `ClassIdSource` with `Cow<'static, str>`
1 parent 3564cfe commit 63b942e

File tree

5 files changed

+23
-85
lines changed

5 files changed

+23
-85
lines changed

godot-codegen/src/generator/classes.rs

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,6 @@ fn make_class(class: &Class, ctx: &mut Context, view: &ApiView) -> GeneratedClas
9191

9292
// Strings
9393
let godot_class_str = &class_name.godot_ty;
94-
let class_name_cstr = util::c_str(godot_class_str);
9594

9695
// Idents and tokens
9796
let (base_ty, base_ident_opt) = match class.base_class.as_ref() {
@@ -248,7 +247,7 @@ fn make_class(class: &Class, ctx: &mut Context, view: &ApiView) -> GeneratedClas
248247
// Optimization note: instead of lazy init, could use separate static which is manually initialized during registration.
249248
static CLASS_ID: std::sync::OnceLock<ClassId> = std::sync::OnceLock::new();
250249

251-
let name: &'static ClassId = CLASS_ID.get_or_init(|| ClassId::__alloc_next_ascii(#class_name_cstr));
250+
let name: &'static ClassId = CLASS_ID.get_or_init(|| ClassId::__alloc_next_unicode(#godot_class_str));
252251
*name
253252
}
254253

godot-core/src/meta/class_id.rs

Lines changed: 18 additions & 71 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@ use std::any::TypeId;
99
use std::borrow::Cow;
1010
use std::cell::OnceCell;
1111
use std::collections::HashMap;
12-
use std::ffi::CStr;
1312
use std::fmt;
1413
use std::hash::Hash;
1514

@@ -95,7 +94,7 @@ impl ClassId {
9594
"In Godot < 4.4, class name must be ASCII: '{name}'"
9695
);
9796

98-
cache.insert_class_id(ClassIdSource::Owned(name), Some(type_id), false)
97+
cache.insert_class_id(Cow::Owned(name), Some(type_id), false)
9998
}
10099

101100
/// Create a `ClassId` from a runtime string (for dynamic class names).
@@ -105,7 +104,7 @@ impl ClassId {
105104
pub(crate) fn new_dynamic(class_name: String) -> Self {
106105
let mut cache = CLASS_ID_CACHE.lock();
107106

108-
cache.insert_class_id(ClassIdSource::Owned(class_name), None, false)
107+
cache.insert_class_id(Cow::Owned(class_name), None, false)
109108
}
110109

111110
// Test-only APIs.
@@ -127,37 +126,16 @@ impl ClassId {
127126
Self { global_index: 0 }
128127
}
129128

130-
/// Create a new ASCII; expect to be unique. Internal, reserved for macros.
131-
#[doc(hidden)]
132-
pub fn __alloc_next_ascii(class_name_cstr: &'static CStr) -> Self {
133-
let utf8 = class_name_cstr
134-
.to_str()
135-
.expect("class name is invalid UTF-8");
136-
137-
assert!(
138-
utf8.is_ascii(),
139-
"ClassId::alloc_next_ascii() with non-ASCII Unicode string '{utf8}'"
140-
);
141-
142-
let source = ClassIdSource::Borrowed(class_name_cstr);
143-
let mut cache = CLASS_ID_CACHE.lock();
144-
cache.insert_class_id(source, None, true)
145-
}
146-
147129
/// Create a new Unicode entry; expect to be unique. Internal, reserved for macros.
148130
#[doc(hidden)]
149131
pub fn __alloc_next_unicode(class_name_str: &'static str) -> Self {
132+
#[cfg(before_api = "4.4")]
150133
assert!(
151-
cfg!(since_api = "4.4"),
134+
class_name_str.is_ascii(),
152135
"Before Godot 4.4, class names must be ASCII, but '{class_name_str}' is not.\nSee https://github.com/godotengine/godot/pull/96501."
153136
);
154137

155-
assert!(
156-
!class_name_str.is_ascii(),
157-
"ClassId::__alloc_next_unicode() with ASCII string '{class_name_str}'"
158-
);
159-
160-
let source = ClassIdSource::Owned(class_name_str.to_owned());
138+
let source = Cow::Borrowed(class_name_str);
161139
let mut cache = CLASS_ID_CACHE.lock();
162140
cache.insert_class_id(source, None, true)
163141
}
@@ -181,7 +159,7 @@ impl ClassId {
181159
pub fn to_cow_str(&self) -> Cow<'static, str> {
182160
let cache = CLASS_ID_CACHE.lock();
183161
let entry = cache.get_entry(self.global_index as usize);
184-
entry.rust_str.as_cow_str()
162+
entry.rust_str.clone()
185163
}
186164

187165
/// The returned pointer is valid indefinitely, as entries are never deleted from the cache.
@@ -198,23 +176,21 @@ impl ClassId {
198176

199177
let string_name = entry
200178
.godot_str
201-
.get_or_init(|| entry.rust_str.to_string_name());
179+
.get_or_init(|| StringName::from(entry.rust_str.as_ref()));
202180

203181
func(string_name)
204182
}
205183
}
206184

207185
impl fmt::Display for ClassId {
208186
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
209-
self.with_string_name(|s| s.fmt(f))
187+
self.to_cow_str().fmt(f)
210188
}
211189
}
212190

213191
impl fmt::Debug for ClassId {
214192
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
215-
let cache = CLASS_ID_CACHE.lock();
216-
let entry = cache.get_entry(self.global_index as usize);
217-
let name = entry.rust_str.as_cow_str();
193+
let name = self.to_cow_str();
218194

219195
if name.is_empty() {
220196
write!(f, "ClassId(none)")
@@ -224,58 +200,31 @@ impl fmt::Debug for ClassId {
224200
}
225201
}
226202

227-
fn ascii_cstr_to_str(cstr: &CStr) -> &str {
228-
cstr.to_str().expect("should be validated ASCII")
229-
}
230-
231203
// ----------------------------------------------------------------------------------------------------------------------------------------------
232204

233205
/// Entry in the class name cache.
234206
///
235207
/// `StringName` needs to be lazy-initialized because the Godot binding may not be initialized yet.
236208
struct ClassIdEntry {
237-
rust_str: ClassIdSource,
209+
rust_str: Cow<'static, str>,
238210
godot_str: OnceCell<StringName>,
239211
}
240212

241213
impl ClassIdEntry {
242-
fn new(rust_str: ClassIdSource) -> Self {
214+
fn new(rust_str: Cow<'static, str>) -> Self {
243215
Self {
244216
rust_str,
245217
godot_str: OnceCell::new(),
246218
}
247219
}
248220

249221
fn none() -> Self {
250-
Self::new(ClassIdSource::Borrowed(c""))
222+
Self::new(Cow::Borrowed(""))
251223
}
252224
}
253225

254226
// ----------------------------------------------------------------------------------------------------------------------------------------------
255227

256-
/// `Cow`-like enum for class names, but with C strings as the borrowed variant.
257-
enum ClassIdSource {
258-
Owned(String),
259-
Borrowed(&'static CStr),
260-
}
261-
262-
impl ClassIdSource {
263-
fn to_string_name(&self) -> StringName {
264-
match self {
265-
ClassIdSource::Owned(s) => StringName::from(s),
266-
ClassIdSource::Borrowed(cstr) => StringName::__cstr(cstr),
267-
}
268-
}
269-
270-
fn as_cow_str(&self) -> Cow<'static, str> {
271-
match self {
272-
ClassIdSource::Owned(s) => Cow::Owned(s.clone()),
273-
ClassIdSource::Borrowed(cstr) => Cow::Borrowed(ascii_cstr_to_str(cstr)),
274-
}
275-
}
276-
}
277-
// ----------------------------------------------------------------------------------------------------------------------------------------------
278-
279228
/// Unified cache for all class name data.
280229
struct ClassIdCache {
281230
/// All class name entries, with index representing [`ClassId::global_index`].
@@ -308,23 +257,21 @@ impl ClassIdCache {
308257
/// If `expect_first` is true and the string is already present in the cache.
309258
fn insert_class_id(
310259
&mut self,
311-
source: ClassIdSource,
260+
source: Cow<'static, str>,
312261
type_id: Option<TypeId>,
313262
expect_first: bool,
314263
) -> ClassId {
315-
let name_str = source.as_cow_str();
316-
317264
if expect_first {
318265
// Debug verification that we're indeed the first to register this string.
319266
#[cfg(debug_assertions)]
320267
assert!(
321-
!self.string_to_index.contains_key(name_str.as_ref()),
268+
!self.string_to_index.contains_key(source.as_ref()),
322269
"insert_class_name() called for already-existing string: {}",
323-
name_str
270+
source
324271
);
325272
} else {
326273
// Check string cache first (dynamic path may reuse existing entries).
327-
if let Some(&existing_index) = self.string_to_index.get(name_str.as_ref()) {
274+
if let Some(&existing_index) = self.string_to_index.get(source.as_ref()) {
328275
// Update type cache if we have a TypeId and it's not already cached (dynamic-then-static case).
329276
// Note: if type_id is Some, we know it came from new_cached_inner after a failed TypeId lookup.
330277
if let Some(type_id) = type_id {
@@ -342,9 +289,9 @@ impl ClassIdCache {
342289
panic!("ClassId cache exceeded maximum capacity of 65536 entries")
343290
});
344291

345-
self.entries.push(ClassIdEntry::new(source));
292+
self.entries.push(ClassIdEntry::new(source.clone()));
346293
self.string_to_index
347-
.insert(name_str.into_owned(), global_index);
294+
.insert(source.into_owned(), global_index);
348295

349296
if let Some(type_id) = type_id {
350297
self.type_to_index.insert(type_id, global_index);

godot-core/src/meta/signature.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -470,7 +470,7 @@ impl<'a> CallContext<'a> {
470470
}
471471

472472
/// Call from Godot into a custom Callable.
473-
pub fn custom_callable(function_name: &'a str) -> Self {
473+
pub const fn custom_callable(function_name: &'a str) -> Self {
474474
Self {
475475
class_name: Cow::Borrowed("<Callable>"),
476476
function_name,

godot-macros/src/class/derive_godot_class.rs

Lines changed: 1 addition & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -55,15 +55,7 @@ pub fn derive_godot_class(item: venial::Item) -> ParseResult<TokenStream> {
5555
.map_or_else(|| class.name.clone(), |rename| rename)
5656
.to_string();
5757

58-
// Determine if we can use ASCII for the class name (in most cases).
59-
// Crate godot-macros does not have knowledge of `api-*` features (and neither does user crate where macros are generated),
60-
// so we can't cause a compile error if Unicode is used before Godot 4.4. However, this causes a runtime error at startup.
61-
let class_name_allocation = if class_name_str.is_ascii() {
62-
let c_str = util::c_str(&class_name_str);
63-
quote! { ClassId::__alloc_next_ascii(#c_str) }
64-
} else {
65-
quote! { ClassId::__alloc_next_unicode(#class_name_str) }
66-
};
58+
let class_name_allocation = quote! { ClassId::__alloc_next_unicode(#class_name_str) };
6759

6860
if struct_cfg.is_internal {
6961
modifiers.push(quote! { with_internal })

itest/rust/src/object_tests/class_name_test.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -158,10 +158,10 @@ fn class_name_debug() {
158158
fn class_name_alloc_panic() {
159159
// ASCII.
160160
{
161-
let _1st = ClassId::__alloc_next_ascii(c"DuplicateTestClass");
161+
let _1st = ClassId::__alloc_next_unicode("DuplicateTestClass");
162162

163163
expect_panic("2nd allocation with same ASCII string fails", || {
164-
let _2nd = ClassId::__alloc_next_ascii(c"DuplicateTestClass");
164+
let _2nd = ClassId::__alloc_next_unicode("DuplicateTestClass");
165165
});
166166
}
167167

0 commit comments

Comments
 (0)