in #[var]s, handle renamed #[func]s
#1019
Open
+339
−12
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
commented
Jan 20, 2025
0x53A
force-pushed
the
dev-handle-func-rename
branch
from
56e5be7 to
e8a3c98
Compare
Bromeon
reviewed
Jan 20, 2025
| @@ -200,10 +201,14 @@ impl GetterSetterImpl { | |||
| } | |||
| } | |||
|
|
|||
| let constant_declaration = | |||
There was a problem hiding this comment.
Suggested change
| 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.
Suggested change
| 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
Comment on lines
352
to
412
| 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?
Suggested change
| 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
Bromeon
reviewed
Jan 21, 2025
| @@ -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. |
Yarwin
reviewed
Jan 23, 2025
0x53A
force-pushed
the
dev-handle-func-rename
branch
from
0965b33 to
7e6fb7a
Compare
…d with (rename=godot_name)
0x53A
force-pushed
the
dev-handle-func-rename
branch
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 get_int_val(&self) -> i32 {
self.int_val
}
#[func(rename=f2)]
pub fn set_int_val(&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 `get_int_val` is registered with Godot as `f1`."]
#[doc(hidden)]
#[allow(non_upper_case_globals)]
pub const get_int_val: &str = "f1";
#[doc = "The Rust function `set_int_val` is registered with Godot as `f2`."]
#[doc(hidden)]
#[allow(non_upper_case_globals)]
pub const set_int_val: &str = "f2";
} |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
closes #863