Generate Into<Option<_>> in argument position where applicable
#1620
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Dec 2, 2024
fengalin
force-pushed
the
more-into-option-args
branch
2 times, most recently
from
d65f584 to
fe98270
Compare
|
I haven't had the time to look into in details, but we would need the same treatment for the generated Builders as well :) |
fengalin
force-pushed
the
more-into-option-args
branch
from
fe98270 to
563ce08
Compare
Ah, that was deliberate, but I get your point! I'll look into this in a bit because the quick fix for that showed another special cased code path. |
Option and references in types were mostly handled in `try_build()` which prevented callers from composing alternative types. This commit redistributes responsabilities between `try_build()` & `try_build_param()` so they actually target their specific use cases: standalone type or function argument (also known as parameter in the code base). These changes are needed to introduce new `BoundType`s in order to generate additional `impl Into<Option<_>>` in argument position. Also: * Update call sites to use the appropriate `try_build()` or `try_build_param()` with required proprties depending on the intention. * Factorised the Unknown Conversion detection, though I suspect it's not really useful and already caught by checking the Unimplemented Type set. * Fixed trampoline `Option<String>` return value incorrectly generated as `Option<GString>` because the `is_gstring()` helper function would not detect the wrapped `GString`. Seen in GES for Project's missing-uri signal. * Add missing parameter config handling for a corner case in return position. * Reworded a couple of identifiers so as to help me better understand the intention. Hopefully, that could also help others. * This incidentally fixed the generation argument references and `Option` in commented out callbacks. While this doesn't improve code in use, it improves the accuracy of generated signatures. * Removed one layer of error reported in some commented function signatures. Some commented out function signatures contained multiple error reports for a single argument, e.g. /*Unknown conversion*//*Unimplemented*/, which added verbosity whithout providing any meaningful insight. Only one is reported now. The last two cause diff in commented methods comparer to previous versions. This was considered not a problem since this will not produce more diffs in subsequent re-generations.
This commit paves the way for the introduction of additional bounds in more contexts by generalizing the use of `Bound` and `to_glib_extra`. `to_glib_extra`s allow prepending a processing before calling the actual conversion to the glib type. For instance, this can be used to convert a value into an `Option` when an `impl Into<Option<_>>` argument is used. This commit adds support for `to_glib_extra` for callback arguments and for property setters. Properties were using a specific `PropertyBound` type which added code duplication and couldn't take advantage of the regular `Bound` which can take advantage of `to_glib_extra`.
fengalin
force-pushed
the
more-into-option-args
branch
from
563ce08 to
74a0115
Compare
This commit adds support for generating `Into<Option<_>>`, `Into<Option<&_>>`
and `Into<Option<&IsA<_>>` in argument position. The existing `analysis::Bound`
type was updated to support new bounds for these variants:
1. Owned value
This is straightforward, just need a `to_glib_extra` `.into()`:
```rust
impl AudioDecoder {
fn finish_frame(&self, buf: impl Into<Option<gst::Buffer>>) -> Result<...> {
[...]
ffi::gst_audio_decoder_finish_frame(
[...]
buf.into().into_glib_ptr(),
)
[...]
}
}
```
2. Concrete types by reference
Same, but needs a lifetime:
```rust
impl TextOverlay {
fn set_text<'a>(&self, text: impl Into<Option<&'a str>>) {
[...]
ffi::ges_text_overlay_set_text()
[...]
text.into().to_glib_none().0,
))
[...]
}
}
```
3. Trait bound by reference
Needs a lifetime and a generic parameter and a longer `to_glib_extra`:
```rust
impl Pipeline {
fn use_clock<'a, P: IsA<Clock>>(&self, clock: impl Into<Option<&'a P>>) {
[...]
ffi::gst_pipeline_use_clock(
[...]
clock.into().as_ref().map(|p| p.as_ref()).to_glib_none().0,
))
[...]
}
}
```
Other Changes:
These changes revealed a bug in trampoline `user_data` generic parameters
handling: these parameters can be grouped, in which case the grouped callbacks
are passed as a tuple in `user_data`. The actual callback types are then
required to recover the callbacks from the tuple. The way it was implemented,
all the callback generic parameters (bounds) from the outter function were
considered as taking part in the `user_data`, regardless of the actual grouping.
From the code bases on which I tested this, this had no consequences since
callbacks for a particular function were all grouped anyway. However, with the
new bounds implemented by this commit, functions with callbacks can now use a
lifetime, which may not be part of the callback signatures, in which case it
should not be included as part of a callback group. This is now fixed. I took
the liberty to add details and remane a couple of identifiers to ease my
understanding of what this code was achieving.
fengalin
force-pushed
the
more-into-option-args
branch
from
74a0115 to
24f0d66
Compare
|
Builder generation updated. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
TryFromGlibPR usedimpl Into<Option<_>>forTryFromGlibarguments which resolve to anOption. This improved usability, e.g.:However, other types were not supported, so we still needed to write e.g.:
Re-generation of the main code bases
This turned out to impact many parts of the generator. I split the result into 3 commits:
RustType,Bound&to_glib_extraand the way they were used (or not used in some code paths). These reworks only impact the commented out generated functions (and fix a shortcoming ingstreamer-rs).The result can be observed as 2 re-generations in:
Into<Option<_>>in argument position where applicable gtk-rs-core#1590Into<Option<_>>in argument position where applicable gtk4-rs#1929See individual commits and specific repository PRs/MR.