Implement #[func] for static functions
#252
Conversation
lilizoey
force-pushed
the
feature/static-func
branch
from
d6a2bd0 to
20bf29b
Compare
|
bors try |
tryBuild succeeded: |
godot-core/src/macros.rs
Outdated
| @@ -315,52 +490,53 @@ macro_rules! gdext_virtual_method_callback_inner { | |||
| /// Returns a C function which acts as the callback when a virtual method of this instance is invoked. | |||
| // | |||
| // Note: code duplicated with gdext_virtual_method_callback | |||
| // there are no static virtual methods from what i can tell. | |||
There was a problem hiding this comment.
This could be stated with a bit more confidence, because what would that even mean? virtual means dynamic polymorphism; the function to be called is selected at runtime based on the runtime type of the self/this pointer. But static by definition does not have one.
There was a problem hiding this comment.
This file is getting a lot of duplication now. Rewriting it as a proc-macro would probably allow us to clean it up a lot. Not for this PR though :)
| var from_gdscript: TYPE = VAL | ||
| var mirrored: TYPE = GenFfi.mirror_static_IDENT(from_gdscript) | ||
| assert_eq(mirrored, from_gdscript, "mirrored_static == from_gdscript") | ||
| #) |
There was a problem hiding this comment.
Excellent idea to test it this way 👍
| fn custom_constructor_works() { | ||
| let obj = object_test_gd::CustomConstructor::construct_object(42); | ||
| assert_eq!(obj.bind().val, 42); | ||
| obj.free(); |
There was a problem hiding this comment.
This will leak if the assert fails. Not a huge deal, but any reason not to use RefCounted as a base instead?
There was a problem hiding this comment.
not really, just kinda what most others used.
| err: *mut sys::GDExtensionCallError, | ||
| func: fn(Self::Params) -> Self::Ret, | ||
| method_name: &str, | ||
| ); |
There was a problem hiding this comment.
In #210, I made the func accept *const sys::GDExtensionConstVariantPtr. This pushes the responsibility of converting from GDExtensionConstVariantPtr to &C, &mut C or nothing at all up to the caller, but allows us to have one varcall and one ptrcall implementation, instead of one for each of the ref, mut and static cases. That cuts down on duplication in this file.
Since you already have a macro that takes "magic words" ref, mut or static it shouldn't be hard to do the same in this PR as well, if we want to.
There was a problem hiding this comment.
it did help a lot with code repetition tbh, so i changed to doing that
Bromeon
added
feature
lilizoey
force-pushed
the
feature/static-func
branch
from
20bf29b to
4404ef0
Compare
| let variant = unsafe { &*(*args_ptr.offset($n) as *mut Variant) }; // TODO from_var_sys | ||
| let arg = <$Pn as FromVariant>::try_from_variant(variant) | ||
| .unwrap_or_else(|e| param_error::<$Pn>(method_name, $n, variant)); | ||
| let args = ($( unsafe { varcall_arg::<$Pn, $n>(args_ptr, method_name) }, )*) ; |
There was a problem hiding this comment.
Could you use the old notation on multiple lines?
Makes it significantly easier to recognize that a tuple is being constructed, and what the value of each element is.
let args = ( $(
unsafe { ... },
)* );Same below.
|
|
||
| let storage = unsafe { crate::private::as_storage::<C>(instance_ptr) }; | ||
| let mut instance = storage.get_mut(); | ||
| let args_ptr = args_ptr as *const *mut Variant; |
There was a problem hiding this comment.
This could probably be moved inside varcall_arg -- the conversion happens at compile time, so it doesn't matter how many elements need to do it.
That way, the access logic is more local, and to the outside we have only an opaque sys pointer.
There was a problem hiding this comment.
varcall_arg isn't public so it wont be visible from the outside. and this way it's clear from the signature that we are intending for this to be a pointer to an actual Variant that we are going to dereference. but i can still do it if you want.
lilizoey
force-pushed
the
feature/static-func
branch
from
4404ef0 to
6b08e83
Compare
lilizoey
force-pushed
the
feature/static-func
branch
from
6b08e83 to
422faba
Compare
|
Thanks a lot! bors r+ |
|
Build succeeded! The publicly hosted instance of bors-ng is deprecated and will go away soon. If you want to self-host your own instance, instructions are here. If you want to switch to GitHub's built-in merge queue, visit their help page.
|
Takes over from #210
Adds support for static methods in classes.
also replaces tabs with spaces in
signature.rs.thanks a lot to @ttencate! it was very straightforward to make this work based on what they'd already done.
from what i can tell, there are no static virtual methods. so we dont need to implement support for that. im not sure if there ever will be but it doesn't seem likely to me at least.
it might be worth it to move more of this logic into procedural macros eventually.
closes #175