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

codegen/function: body future handles ownership of slice argument #1580

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

Conversation

BiagioFesta
Copy link

Currently, the future body generated is something like:

pub fn probe_endpoints_future(
    my_slice: &[MyObject],
) -> Pin<
    Box_<dyn std::future::Future<Output = Result<_, glib::Error>> + 'static>,
> {
    skip_assert_initialized!();
   
   let my_slice = my_slice.map(ToOwned::to_owned);   // if my_slice is declared as nullable
             // or
   let my_slice = my_slice.clone();                  // if my_slice is not declared as nullable
   


    Box_::pin(gio::GioFuture::new(&(), move |_obj, cancellable, send| {
      // ...
}

both approaches fail to compile as:

  • .map does not exist for slice type (it is not an Option)
  • .clone() does not really give ownership of the data slice, as clone only copies the wide-pointer.
    • This is an issue as the future closure need to move something which needs 'static lifetime (and that's the reason why the code in the generator performs a clone or to_owned in the first place).

This PR aims to handle the case of slice argument

@BiagioFesta
Copy link
Author

CI is failing, but I don't think it is related with the changes:

Resolving github.com (github.com)... 140.82.116.4
Connecting to github.com (github.com)|140.82.116.4|:443... connected.
HTTP request sent, awaiting response... 404 Not Found
2024-06-12 15:30:35 ERROR 404: Not Found


if *c_par.nullable {
if is_slice {
Copy link
Member

Choose a reason for hiding this comment

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

Would also support a nullable slice while at it :)

Copy link
Author

Choose a reason for hiding this comment

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

AFAIK, nullable annotation on array still produces a &[] (and not a Option<&[]>). So to_vec still applies

Copy link
Member

Choose a reason for hiding this comment

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

AFAIK, nullable annotation on array still produces a &[] (and not a Option<&[]>). So to_vec still applies

That is an issue that should be fixed separately, see #1551

Copy link
Author

Choose a reason for hiding this comment

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

However, that PR is still not merged.

If I change my code assuming I can have Option<&[]>, that is:

if *c_par.nullable {
   ...
} else if (is_slice) {
   ...
} ...

I'd end up still having a bug generated code until that PR kicks in.

Isn't it better to change the ifs order in #1551 where the logic actually changes?


(P.S: why having Option<&[]>. That's not rust idiomatic. the current behavior (if I am not mistaken): &[] even when array is annotated as nullable makes sense in the generated rust type

Copy link
Member

Choose a reason for hiding this comment

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

Sometimes there is a difference in behaviour between passing None and &[]. See #1133 (comment) but agree, that can be done in the other PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants