Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make Var use GodotConvert::Via to align it with ToGodot and FromGodot #595

Merged
merged 1 commit into from
Feb 6, 2024

Conversation

lilizoey
Copy link
Member

@lilizoey lilizoey commented Feb 5, 2024

Var now depends on GodotConvert and uses GodotConvert::Via instead of its own intermediate type

Var does not depend on ToGodot or FromGodot to make it possible to have properties that cannot be used as function arguments/return types. This is currently the case for OnReady<T>. The current implementations do however use ToGodot and FromGodot to perform the conversions.

This also required some changes in the GodotConvert and Var macros to make sure they agree on the Via type to use for structs/enums.

I didn't change the derive macros much beyond making them compatible with each other, as i think they could do with a bigger rewrite later, see for instance #452.

closes #453

@lilizoey lilizoey added quality-of-life No new functionality, but improves ergonomics/internals c: register Register classes, functions and other symbols to GDScript labels Feb 5, 2024
@GodotRust
Copy link

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-595

Copy link
Member

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Nice to see that some parts get even simpler 🙂

godot-core/src/obj/onready.rs Outdated Show resolved Hide resolved
godot-core/src/property.rs Show resolved Hide resolved
godot-macros/src/lib.rs Outdated Show resolved Hide resolved
godot-macros/src/util/mod.rs Show resolved Hide resolved
godot-macros/src/util/mod.rs Outdated Show resolved Hide resolved
godot-macros/src/util/mod.rs Outdated Show resolved Hide resolved
Comment on lines 332 to 334
// `repr` is always going to look like `#[repr()]`
unreachable!()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is unreachable -- a user could still specify #[repr = u32] or something 🤔

So might be better to bail here...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any form of #[repr] that doesn't look like #[repr()] will give a rust compile time error:

#[repr = i64] // attribute value must be a literal
#[repr = "i64"] // malformed `repr` attribute input
#[repr] // malformed `repr` attribute input
#[repr(i64, i32)] // conflicting representation hints, this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!

however #[repr()] is just a warning apparently:

#[repr()] // unused attribute, attribute `repr` with an empty list has no effect 

which surprised me a bit tbh, i'll fix that then ig,

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only thing left, otherwise I think it's ready 🙂

Comment on lines 336 to 340
let TokenTree::Ident(repr_type) = &repr_value[0] else {
unreachable!()
};
Copy link
Member

@Bromeon Bromeon Feb 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also here, what if the user specifies #[repr()] or #[repr(+)]?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#[repr(+)] // expected unsuffixed literal or identifier, found `+`

itest/rust/src/object_tests/property_test.rs Outdated Show resolved Hide resolved
@lilizoey lilizoey force-pushed the fix/godot-convert-var branch 3 times, most recently from 80011ed to 71f55d3 Compare February 6, 2024 16:16
@lilizoey lilizoey force-pushed the fix/godot-convert-var branch from 71f55d3 to 79df2b4 Compare February 6, 2024 16:24
@Bromeon Bromeon added this pull request to the merge queue Feb 6, 2024
@Bromeon
Copy link
Member

Bromeon commented Feb 6, 2024

Thank you!

Merged via the queue into godot-rust:master with commit 9aaea3c Feb 6, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: register Register classes, functions and other symbols to GDScript quality-of-life No new functionality, but improves ergonomics/internals
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Integrate Property with the GodotConvert API
3 participants