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

generate nullable arrays as options #1136

Closed
wants to merge 1 commit into from

Conversation

euclio
Copy link
Contributor

@euclio euclio commented May 14, 2021

Fixes #1133.

First time contributor, would appreciate some pointers on how to test this.

@sdroege
Copy link
Member

sdroege commented May 14, 2021

First time contributor, would appreciate some pointers on how to test this.

You could take the gtk-rs-core repo and run generator.py with your gir branch, then create a WIP PR with that. We'd review that PR together with this one to make sure that then generated output is also correct.

Thanks for working on this :)

let nullable = function_parameters
.iter()
.find(|p| p.name == array_name)
.unwrap()
Copy link
Member

Choose a reason for hiding this comment

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

This panics when running ./generator.py --gir-path path/to/gir while generating code for glib:

[...]
[WARN  libgir::analysis::functions] `g_variant_new_from_data`: destructor without linked callback
thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', src/analysis/function_parameters.rs:255:18
stack backtrace:
   0: rust_begin_unwind
             at /rustc/88f19c6dab716c6281af7602e30f413e809c5974/library/std/src/panicking.rs:493:5
   1: core::panicking::panic_fmt
             at /rustc/88f19c6dab716c6281af7602e30f413e809c5974/library/core/src/panicking.rs:92:14
   2: core::panicking::panic
             at /rustc/88f19c6dab716c6281af7602e30f413e809c5974/library/core/src/panicking.rs:50:5
   3: libgir::analysis::function_parameters::analyze
   4: libgir::analysis::functions::analyze_function
   5: libgir::analysis::functions::analyze
   6: libgir::analysis::run
   7: gir::main
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

Copy link
Member

Choose a reason for hiding this comment

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

Line 239 to 247 try to get the array name in various ways. It would probably be best to directly return the nullable in those places.

@MarijnS95
Copy link
Contributor

MarijnS95 commented May 14, 2021

We discussed a similar problem very recently for the Ash (Vulkan wrapper) crate. Theoretically a zero-length slice (&[]) would work for this too as implementations aren't supposed to dereference a pointer when the length field is set to zero: what would they expect to read otherwise (or write, in the case of output)? That way you could simply set the pointer to ptr::null() if slice.is_empty().

However, on the other hand this might more clearly signal that an array is indeed optional and not hidden behind a zero-length slice "trick". Complementing that, codegen might assert that slices are never empty? It's a complicated and ambiguous subject :/

@euclio
Copy link
Contributor Author

euclio commented May 14, 2021

It's tricky. I think Option<&[u8]> makes more sense because that lets you distinguish between no array at all vs. empty array, regardless of what the implementation does.

@euclio
Copy link
Contributor Author

euclio commented Nov 5, 2021

@sdroege Finally found some time to get back to this. I've fixed the crash.

Changes are visible on my gtk-rs-core fork.

@sdroege
Copy link
Member

sdroege commented Nov 9, 2021

This needs some more changes:

error[E0599]: the method `map` exists for reference `&[&Atom]`, but its trait bounds were not satisfied
Error:    --> gdk/src/auto/display.rs:271:33
    |
271 |         let n_targets = targets.map(|arr| arr.len()).unwrap_or(0) as i32;
    |                                 ^^^ method cannot be called on `&[&Atom]` due to unsatisfied trait bounds
    |
    = note: the following trait bounds were not satisfied:
            `&[&Atom]: Iterator`
            which is required by `&mut &[&Atom]: Iterator`
            `[&Atom]: Iterator`
            which is required by `&mut [&Atom]: Iterator`

@euclio
Copy link
Contributor Author

euclio commented Nov 9, 2021

Tracked it down to this line. Removing it outright is not correct, because that will cause functions like list_axes to return an Option<Vec<_>> instead of Vec<_>.

@sdroege
Copy link
Member

sdroege commented Nov 10, 2021

It will have to check if the return value is nullable there, I guess. For GList / GSList this is never the case btw (an empty list is literally NULL).

@euclio
Copy link
Contributor Author

euclio commented Jan 5, 2022

I'm going to close this for now since I don't know when I'll have time to get back to it, and it doesn't affect me anymore.

Anyone seeing this should feel free to continue where I left off.

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.

data parameter for gio::content_type_guess should be an Option
3 participants