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.

Sign up for GitHub

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

Jump to bottom

Require AsObjectArg pass-by-ref, consistent with AsArg #947

Merged
merged 6 commits into from
Nov 10, 2024
Merged

Require AsObjectArg pass-by-ref, consistent with AsArg #947

merged 6 commits into from
Nov 10, 2024

Commits on Nov 9, 2024

  1. save() and try_save() take Gd<T> by-ref

    @Bromeon
    Bromeon committed Nov 9, 2024
    Configuration menu
    Copy the full SHA
    5b8df20 View commit details
    Browse the repository at this point in the history

  2. Hide some API symbols

    @Bromeon
    Bromeon committed Nov 9, 2024
    Configuration menu
    Copy the full SHA
    d570536 View commit details
    Browse the repository at this point in the history

  3. Remove impl AsObjectArg<T> for Gd<T> values 

    The idea is to require pass-by-ref for objects, in order to discourage
accidental/unnecessary cloning. This is also consistent with AsArg
by-refs, in particular `impl AsArg<Gd<T>>` when used in arrays and
packed arrays.

The downside is that it needs an extra `&` in some places, and one can
no longer use pass-by-value to "consume" objects, ending their lifetime
with the call. What is also unclear is whether there are situations
where we can benefit from moving "into" the engine (perf-wise). So this
may need some more discussion.

One option is also to move toward using Copy for manually managed Gd<T>
types, which would mean a) those could be passed by-value, and b) we
would anyway lose the "consume" semantics.

This is a more conservative choice for now, with minor syntactic
drawbacks, but in line with AsArg<T> and the potential to expand in the
future.
    @Bromeon
    Bromeon committed Nov 9, 2024
    Configuration menu
    Copy the full SHA
    7bea565 View commit details
    Browse the repository at this point in the history

  4. Remove impl AsObjectArg<T> for &mut Gd<T> 

    `&mut obj` args can only be passed once, since &mut T: !Copy. This means
that passing it to a Godot API works once, and then causes moved-out
errors. This comes with a few issues:

* It either needs the `&*` pattern or not, depending on the order of
  calls. Reordering breaks them.

* It's inconsistent with `AsArg`, which is only implemented for `&T` in
  case of by-ref builtins.

* Consuming `&mut T` should probably be allowed iff consuming `T` is
  allowed.

That said, we'll need to check the ergonomic impact and see how
arguments are being passed in practice.
    @Bromeon
    Bromeon committed Nov 9, 2024
    Configuration menu
    Copy the full SHA
    af53e74 View commit details
    Browse the repository at this point in the history

  5. Discussion about impl AsObjectArg<T> for &Option<Gd<T>> (outer ref) 

    It's relatively common that Godot APIs return `Option<Gd<T>>` or pass
this type in virtual functions. To avoid excessive `as_ref()` calls, we
**could** directly support `&Option<Gd>` in addition to `Option<&Gd>`.

However, this is currently not done as it hides nullability, especially
in situations where a return type is directly propagated:

    api(create_obj().as_ref())
    api(&create_obj())

While the first is slightly longer, it looks different from a function
create_obj() that returns Gd<T> and thus can never be null. It's also
quite idiomatic to use as_ref() for inner-option transforms in Rust.

For now, the code is still added but inactive.
    @Bromeon
    Bromeon committed Nov 9, 2024
    Configuration menu
    Copy the full SHA
    b9b81ed View commit details
    Browse the repository at this point in the history

  6. Document AsObjectArg better

    @Bromeon
    Bromeon committed Nov 9, 2024
    Configuration menu
    Copy the full SHA
    1110e37 View commit details
    Browse the repository at this point in the history