in #[var]s, handle renamed #[func]s
#1019
Conversation
0x53A
force-pushed
the
dev-handle-func-rename
branch
from
7d0f540 to
6695a62
Compare
|
API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-1019 |
0x53A
force-pushed
the
dev-handle-func-rename
branch
from
6695a62 to
56e5be7
Compare
0x53A
force-pushed
the
dev-handle-func-rename
branch
from
56e5be7 to
e8a3c98
Compare
| @@ -200,10 +201,14 @@ impl GetterSetterImpl { | |||
| } | |||
| } | |||
|
|
|||
| let constant_declaration = | |||
There was a problem hiding this comment.
| let constant_declaration = | |
| let func_name_constant = |
To make clear that it's not a user-defined #[constant].
| @@ -84,6 +86,8 @@ pub fn transform_inherent_impl( | |||
| let (funcs, signals) = process_godot_fns(&class_name, &mut impl_block, meta.secondary)?; | |||
| let consts = process_godot_constants(&mut impl_block)?; | |||
|
|
|||
| let func_export_name_constants = make_function_registered_name_constants(&funcs, &class_name); | |||
There was a problem hiding this comment.
| let func_export_name_constants = make_function_registered_name_constants(&funcs, &class_name); | |
| let func_name_constants = make_function_registered_name_constants(&funcs, &class_name); |
"export" has a very specific meaning; using it here is probably confusing.
There was a problem hiding this comment.
it was also not consistent with the function name. I'd propose standardizing on "registered" function name, at least that's what it's called in other places in the code.
There was a problem hiding this comment.
It's fine if the local variables are more terse/concise than the function names. Context matters, and if you see the
let func_name_constants = make_function_registered_name_constants(...);once, then you know what func_name_constants refers to. I think using overly long variable names makes expressions harder to read; the "registered" doesn't really add useful information to understand the codegen.
There's also the point of scope: global function names are visible in their entire module, while variable names are limited to the block/function in which they are declared. It's much easier to mentally keep track of a block, than it is of a module; and there's less potential for collisions.
godot-macros/src/util/mod.rs
Outdated
| pub fn format_function_registered_name_constant(class_name: &Ident, func_name: &Ident) -> Ident { | ||
| format_ident!("__gdext_func_{}_{}", class_name, func_name) | ||
| } |
There was a problem hiding this comment.
I'd use consistent naming, that makes it obvious you're referring to the same thing.
Above I suggested func_name_constant, maybe that can be done here, too?
| pub fn format_function_registered_name_constant(class_name: &Ident, func_name: &Ident) -> Ident { | |
| format_ident!("__gdext_func_{}_{}", class_name, func_name) | |
| } | |
| pub fn format_func_name_constant(class_name: &Ident, func_name: &Ident) -> Ident { | |
| format_ident!("__gdext_func_{class_name}_{func_name}") | |
| } |
|
proposal: to not pollute the struct namespace with all these constants, I could create a dummy struct. Currently the following code is generated: // #[derive(GodotClass)]
impl O {
pub fn get_int_val(&self) -> <i32 as ::godot::meta::GodotConvert>::Via {
<i32 as ::godot::register::property::Var>::get_property(&self.int_val)
}
#[doc(hidden)]
#[allow(non_upper_case_globals)]
pub const __gdext_func_O_get_int_val: &str = "get_int_val";
pub fn set_int_val(&mut self, int_val: <i32 as ::godot::meta::GodotConvert>::Via) {
<i32 as ::godot::register::property::Var>::set_property(&mut self.int_val, int_val);
}
#[doc(hidden)]
#[allow(non_upper_case_globals)]
pub const __gdext_func_O_set_int_val: &str = "set_int_val";
}
// #[godot_api]
impl O {
#[doc(hidden)]
#[allow(non_upper_case_globals)]
pub const __gdext_func_O_foo: &str = "f1";
#[doc(hidden)]
#[allow(non_upper_case_globals)]
pub const __gdext_func_O_bar: &str = "f2";
}I would propose the following: // #[derive(GodotClass)]
impl O {
pub fn get_int_val(&self) -> <i32 as ::godot::meta::GodotConvert>::Via {
<i32 as ::godot::register::property::Var>::get_property(&self.int_val)
}
pub fn set_int_val(&mut self, int_val: <i32 as ::godot::meta::GodotConvert>::Via) {
<i32 as ::godot::register::property::Var>::set_property(&mut self.int_val, int_val);
}
}
#[doc(hidden)]
struct O_Functions { }
impl O_Functions {
#[doc(hidden)]
#[allow(non_upper_case_globals)]
pub const __gdext_func_O_set_int_val: &str = "set_int_val";
#[doc(hidden)]
#[allow(non_upper_case_globals)]
pub const __gdext_func_O_get_int_val: &str = "get_int_val";
}
// #[godot_api]
impl O_Functions {
#[doc(hidden)]
#[allow(non_upper_case_globals)]
pub const __gdext_func_O_foo: &str = "f1";
#[doc(hidden)]
#[allow(non_upper_case_globals)]
pub const __gdext_func_O_bar: &str = "f2";
}The dummy struct is required instead of a |
|
alright no more time for right now, I need to go. I'll look through it one more time when I'm back, then I'll rebase-squash. Todo:
|
|
regarding the memory leak, the memory is allocated here: https://github.com/godotengine/godot/blob/1b7b009674e05b566be11a5377b935f5d3d6c0ee/core/extension/gdextension.cpp#L509 and I could see it being leaked, if it returns with an error here, in case the method name is duplicated: https://github.com/godotengine/godot/blob/master/core/object/class_db.cpp#L1881 why would the method name be duplicated? no idea |
|
After looking into this, I believe it's a bug in godot: godotengine/godot#101870 It's triggered because in mod.rs#244, it doesn't handle pub(crate) fn extract_cfg_attrs(
attrs: &[venial::Attribute],
) -> impl IntoIterator<Item = &venial::Attribute> {
attrs.iter().filter(|attr| {
attr.get_single_path_segment()
.is_some_and(|name| name == "cfg")
})
}and I added a test case that uses cfg_attr, therefore the method is registered multiple times with godot, hitting the memory leak there |
0x53A
changed the title
#[var]s, handle renamed #[func]s#[var]s, handle renamed #[func]s
|
Quick tip: instead of renaming the title to |
Bromeon
added
bug
c: register
| @@ -84,6 +86,8 @@ pub fn transform_inherent_impl( | |||
| let (funcs, signals) = process_godot_fns(&class_name, &mut impl_block, meta.secondary)?; | |||
| let consts = process_godot_constants(&mut impl_block)?; | |||
|
|
|||
| let func_export_name_constants = make_function_registered_name_constants(&funcs, &class_name); | |||
There was a problem hiding this comment.
It's fine if the local variables are more terse/concise than the function names. Context matters, and if you see the
let func_name_constants = make_function_registered_name_constants(...);once, then you know what func_name_constants refers to. I think using overly long variable names makes expressions harder to read; the "registered" doesn't really add useful information to understand the codegen.
There's also the point of scope: global function names are visible in their entire module, while variable names are limited to the block/function in which they are declared. It's much easier to mentally keep track of a block, than it is of a module; and there's less potential for collisions.
| @@ -119,6 +125,32 @@ pub fn transform_inherent_impl( | |||
| }); | |||
| }; | |||
|
|
|||
| let class_functions_name = format_function_registered_name_struct_name(&class_name); | |||
| let class_functions_path: Path = match impl_block.self_ty.as_path().unwrap().segments.as_slice() | |||
There was a problem hiding this comment.
The way you use unwrap here indicates that it's infallible. In general, this would need a brief comment documenting why.
Here, it might be better to pass in previously evaluated values via parameters, making the extraction unnecessary. This especially holds true here since as_path() isn't cheap to compute, even less so with some upcoming validations in venial.
Might also make sense to extract the match expression into its own variable 🙂
That sounds like a good idea! |
i feel like it should be prefixed with |
Good point, in that case we could maybe even drop the prefix of the function constants themselves, as they're now scoped. |
|
It's already implemented, the naming patterns are here: gdext/godot-macros/src/util/mod.rs Lines 372 to 383 in eef5faf Functionally, the PR is done, I just need to implement all the feedback and simplify two places I've hacked together. |
0x53A
force-pushed
the
dev-handle-func-rename
branch
2 times, most recently
from
7e6fb7a to
74fab79
Compare
0x53A
changed the title
#[var]s, handle renamed #[func]s#[var]s, handle renamed #[func]s
|
alright took a little more time but should now be ready. I hope I caught all the review comments w.r.t. naming. It copies over both As mentioned before, it generates the code below. #[derive(GodotClass)]
#[class(base=Node, init)]
struct RenamedFunc {
#[var(get, set)]
int_val: i32,
}
#[godot_api]
impl RenamedFunc {
#[func(rename=f1)]
pub fn foo(&self) -> i32 {
self.int_val
}
#[func(rename=f2)]
pub fn bar(&mut self, val: i32) {
self.int_val = val;
}
}// Recursive expansion of GodotClass macro
// ========================================
#[doc(hidden)]
pub struct __gdext_RenamedFunc_Functions {}
// ...
impl __gdext_RenamedFunc_Functions {
#[doc = "The Rust function `get_int_val` is registered with Godot as `get_int_val`."]
#[doc(hidden)]
#[allow(non_upper_case_globals)]
pub const get_int_val: &str = "get_int_val";
#[doc = "The Rust function `set_int_val` is registered with Godot as `set_int_val`."]
#[doc(hidden)]
#[allow(non_upper_case_globals)]
pub const set_int_val: &str = "set_int_val";
}
// ...
// Recursive expansion of godot_api macro
// =======================================
// ...
impl __gdext_RenamedFunc_Functions {
#[doc = "The Rust function `foo` is registered with Godot as `f1`."]
#[doc(hidden)]
#[allow(non_upper_case_globals)]
pub const foo: &str = "f1";
#[doc = "The Rust function `bar` is registered with Godot as `f2`."]
#[doc(hidden)]
#[allow(non_upper_case_globals)]
pub const bar: &str = "f2";
} |
…d with (rename=godot_name)
0x53A
force-pushed
the
dev-handle-func-rename
branch
from
d005e7e to
2615c97
Compare
| @@ -89,6 +93,19 @@ pub fn transform_inherent_impl( | |||
| #[cfg(not(all(feature = "register-docs", since_api = "4.3")))] | |||
| let docs = quote! {}; | |||
|
|
|||
| // This is the container struct that holds the names of all registered #[func]s. | |||
| // (The struct is declared by the macro derive_godot_class.) | |||
There was a problem hiding this comment.
in most cases macros are being refereed by their user-side representation such as #[derive(GodotClass)] or #[godot_api] instead of macro derive_godot_class. Should we keep this convention?
There was a problem hiding this comment.
also in other place it is being called "derive macro GodotClass" 😅
| @@ -89,6 +93,19 @@ pub fn transform_inherent_impl( | |||
| #[cfg(not(all(feature = "register-docs", since_api = "4.3")))] | |||
| let docs = quote! {}; | |||
|
|
|||
| // This is the container struct that holds the names of all registered #[func]s. | |||
There was a problem hiding this comment.
This is the container struct that holds the names of all registered #[func]s.
Container struct holding names of all registered #[func]s.
?
| @@ -89,6 +93,19 @@ pub fn transform_inherent_impl( | |||
| #[cfg(not(all(feature = "register-docs", since_api = "4.3")))] | |||
| let docs = quote! {}; | |||
|
|
|||
| // This is the container struct that holds the names of all registered #[func]s. | |||
| // (The struct is declared by the macro derive_godot_class.) | |||
| let class_functions_name = format_func_name_struct_name(&class_name); | |||
There was a problem hiding this comment.
tbh it would be good to add comment over this section to specify what is going on; something along the lines of // handle class renaming
| ))?; | ||
| let class_functions_path: Path = | ||
| replace_class_in_path(this_class_full_path, class_functions_name); | ||
| // For each #[func] in this impl block, we create one constant. |
There was a problem hiding this comment.
Kinda noisy; personally I would rather see proper docstring for make_func_name_constants .
Also why is it named make_func_name_constants when it expects funcs as an argument 🤔?
There was a problem hiding this comment.
Also why is it named make_func_name_constants when it expects funcs as an argument 🤔?
One constant is a func_name_constant, multiple constants are func_name_constant[s], at least that was my logic.
| #hint, | ||
| #usage_flags, | ||
| ); | ||
| }); | ||
| } | ||
|
|
||
| // For each generated #[func], add a const. | ||
| // This is the name of the container struct, which is declared by the derive macro GodotClass. |
| let this_class_full_path = impl_block.self_ty.as_path().ok_or(error_fn( | ||
| "unexpected: the function already checked 'as_path' above in validate_impl", | ||
| &impl_block, | ||
| ))?; |
There was a problem hiding this comment.
ok_or_else would be better here, as it only evaluates error_fn() in the error case.
But it might also be more straightforward to just use let Some(...) else { ... }.
| let Some(attr_name) = attr.get_single_path_segment() else { | ||
| return false; | ||
| }; | ||
| // #[cfg(condition)] | ||
| if attr_name == "cfg" { | ||
| return true; | ||
| } | ||
| // #[cfg_attr(condition, attributes...)], note that there can be multiple attributes seperated by comma. | ||
| if attr_name == "cfg_attr" && attr.value.to_token_stream().to_string().contains("cfg(") { | ||
| return true; | ||
| } | ||
| false |
There was a problem hiding this comment.
Please add empty lines before comments that document multi-line statements:
| let Some(attr_name) = attr.get_single_path_segment() else { | |
| return false; | |
| }; | |
| // #[cfg(condition)] | |
| if attr_name == "cfg" { | |
| return true; | |
| } | |
| // #[cfg_attr(condition, attributes...)], note that there can be multiple attributes seperated by comma. | |
| if attr_name == "cfg_attr" && attr.value.to_token_stream().to_string().contains("cfg(") { | |
| return true; | |
| } | |
| false | |
| let Some(attr_name) = attr.get_single_path_segment() else { | |
| return false; | |
| }; | |
| // #[cfg(condition)] | |
| if attr_name == "cfg" { | |
| return true; | |
| } | |
| // #[cfg_attr(condition, attributes...)], note that there can be multiple attributes seperated by comma. | |
| if attr_name == "cfg_attr" && attr.value.to_token_stream().to_string().contains("cfg(") { | |
| return true; | |
| } | |
| false |
| // Can't happen, you have at least one segment (the class name). | ||
| [] => panic!("unexpected: empty path"), |
There was a problem hiding this comment.
If it can't happen, should be unreachable! instead of panic!.
| @@ -164,6 +181,9 @@ pub fn transform_inherent_impl( | |||
| #trait_impl | |||
| #fill_storage | |||
| #class_registration | |||
| impl #class_functions_path { | |||
| #( #func_name_constants )* | |||
There was a problem hiding this comment.
| #( #func_name_constants )* | |
| #( #func_name_constants )* |
Also below
closes #863