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

Unify string parameters across the repo #1037

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

chitoyuu
Copy link
Contributor

The types of string parameters across the repo has been regularized according to the following rules that shall be followed from now on:

  • Methods during the init/registration process should take &str for things like identifiers. They may specify explicit lifetimes in their types if necessary, but not 'static.
  • Manually written methods that abstract over, or mimic API methods should take impl Into<GodotString> or impl Into<NodePath> depending on the usage, to be consistent with their generated counterparts.

Regarding the usage of &str in init instead of more specific conversions required by each method:

The GDNative API isn't very consistent when it comes to the string types it wants during init. Sometimes, functions may want different types of strings even when they are concepturally similar, like for signal and method registration. This gets more complicated when we add our own library-side logic into the mix, like for the InitHandle::add_class family, where allocation is current inevitable even when they are given &'static strs.

It still makes sense to avoid excessive allocation, but that should not get in the way of future development. 'static in particular is extremely limiting, and there are very few cases of its usage throughout the library's history that we have not since regretted. It shouldn't be the norm but the rare exception.

In any case, class registration is something that only run once in the lifecycle of a game, and for the amount of classes most people are using with gdnative, we can afford to be slightly inefficient with strings to make our APIs more consistent, less leaky, and easier to stablize and maintain.

Close #996

@chitoyuu chitoyuu added c: core Component: core (mod core_types, object, log, init, ...) c: export Component: export (mod export, derive) breaking-change Issues and PRs that are breaking to fix/merge. quality-of-life No new functionality, but improves ergonomics/internals labels Feb 22, 2023
@chitoyuu chitoyuu added this to the v0.12.0 milestone Feb 22, 2023
The types of string parameters across the repo has been regularized
according to the following rules that shall be followed from now on:

- Methods during the init/registration process should take `&str` for
  things like identifiers. They may specify explicit lifetimes in their
  types if necessary, but *not* `'static`.
- Manually written methods that abstract over, or mimic API methods should
  take `impl Into<GodotString>` or `impl Into<NodePath>` depending on the
  usage, to be consistent with their generated counterparts.

Regarding the usage of `&str` in init instead of more specific conversions
required by each method:

The GDNative API isn't very consistent when it comes to the string types
it wants during init. Sometimes, functions may want different types of
strings even when they are concepturally similar, like for signal and
method registration. This gets more complicated when we add our own
library-side logic into the mix, like for the `InitHandle::add_class`
family, where allocation is current inevitable even when they are given
`&'static str`s.

It still makes sense to avoid excessive allocation, but that should not
get in the way of future development. `'static` in particular is
extremely limiting, and there are very few cases of its usage throughout
the library's history that we have not since regretted. It shouldn't be
the norm but the rare exception.

In any case, class registration is something that only run once in the
lifecycle of a game, and for the amount of classes most people are using
with `gdnative`, we can afford to be slightly inefficient with strings
to make our APIs more consistent, less leaky, and easier to stablize and
maintain.
@chitoyuu chitoyuu force-pushed the feature/unified-string-parameters branch from 92eaeed to 6f58d75 Compare February 22, 2023 11:53
@Bromeon
Copy link
Member

Bromeon commented Feb 22, 2023

That's something that has always bothered me, very nice to see that you came up with guidelines to unify string usage! 👍


In any case, class registration is something that only run once in the lifecycle of a game, and for the amount of classes most people are using with gdnative, we can afford to be slightly inefficient with strings to make our APIs more consistent, less leaky, and easier to stablize and maintain.

A similar argument could maybe be brought for error types like FromVariantError::InvalidInstance, which currently hold a Cow 🐮

While they do not occur only once like registration, they should ideally happen rarely (although one could maybe more dynamic imagine use cases where these are more frequent). Maybe it's not as pressing, since the Cow<'static, str> does not propagate the lifetime up to its surrounding enum. Or how do you see this?

@chitoyuu
Copy link
Contributor Author

A similar argument could maybe be brought for error types like FromVariantError::InvalidInstance, which currently hold a Cow cow

While they do not occur only once like registration, they should ideally happen rarely (although one could maybe more dynamic imagine use cases where these are more frequent). Maybe it's not as pressing, since the Cow<'static, str> does not propagate the lifetime up to its surrounding enum. Or how do you see this?

Indeed. Thanks for pointing it out! I think it should indeed be considered as a separate case, and perhaps also dealt with within this PR. Some of the variants of FromVariantError contain &'static str as well, which I've personally ran into issues with for one of those "more dynamic" use cases in a project.

There is a minor point that errors like FromVariantError don't always strictly belong to the error path. They can be constructed e.g. in the process of deserializing an untagged enum, of which a smaller example can be seen in the Option<T> impl. I don't think it should be that common for the "heavy" variants to be involved this way though.

I guess it's a fair starting point to just make them all Strings, or we can encapsulate everything (probably with the help of some macros) so we're free to change the details later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Issues and PRs that are breaking to fix/merge. c: core Component: core (mod core_types, object, log, init, ...) c: export Component: export (mod export, derive) quality-of-life No new functionality, but improves ergonomics/internals
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unify string parameters across all builders
2 participants