Skip to content

Conversation

@MarijnS95
Copy link
Contributor

@MarijnS95 MarijnS95 commented May 23, 2021

For gtk-rs/gtk-rs-core#89 (comment)


impl is quite a bit more concise and readable when a type parameter with trait bound is only used once. Aside that, this simplifies some of the type handling in trampolines by using P: IsA<> directly instead of through a where P: ... indirection.


Related MRs:
https://gitlab.freedesktop.org/gstreamer/gstreamer-rs/-/merge_requests/778
gtk-rs/gtk-rs-core#95
gtk-rs/gtk3-rs#547
gtk-rs/gtk4-rs#404

@MarijnS95
Copy link
Contributor Author

So this is far less trivial than anticipated, and not how I planned to spend my sunday!

There's still some trouble with trampolines now that I made aliases optional but the where TYPE_PARAMETERS_START: IsA is hardcoded anyway (for traits) so that's what I'll replace the TODO with.

Other than that the current "heuristic" to detect whether to use impl is based on NoWrapper, which appears to be an explicit type-name/trait-bound. If it's not that we'll use impl instead, assuming the type-name isn't used elsewhere in the code. That should probably be enough.

I'll be out for a while, but feel more than welcome to go over the TODOs and give suggestions (keeping the above in mind, as I have plans for those TODOs already).

Comment on lines +34 to 40
// TODO: This is just a heuristic for now, based on what we do in codegen!
// Theoretically the surrounding function should determine whether it needs to
// reuse an alias (ie. to use in `call_func::<P, Q, R>`) or not.
// In the latter case an `impl` is generated instead of a type name/alias.
pub fn has_alias(&self) -> bool {
matches!(*self, Self::NoWrapper)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Anyone have a suggestion how to determine this?

Comment on lines -39 to +48
pub alias: char,
/// Bound does not have an alias when `param: impl type_str` is used
pub alias: Option<char>,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What this change is all about :)

Comment on lines -286 to +254
.filter(|bound| skip.contains(&bound.alias))
// TODO: False or true?
.filter(|bound| bound.alias.map_or(false, |alias| skip.contains(&alias)))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note to self: Try and remember what was up here.

Comment on lines 265 to 270
let bounds = bounds.iter().filter(|bound| {
bound.alias.map_or(true, |alias| !skip.contains(&alias))
&& (!filter_callback_modified || !bound.callback_modified)
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need to revisit this: can't be a trait bound if it doesn't have an alias, then it's only written in-place with impl.

Comment on lines +284 to +292
// TODO: enforce that this is only used on NoWrapper!
// TODO: Analyze if alias is used in function, otherwise set to None!
.chain(bounds.filter_map(|b| b.alias).map(|a| a.to_string()))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ambiguity between NoWrapper and impl (no alias) again.

MarijnS95 added a commit to MarijnS95/gtk-rs-core that referenced this pull request Jul 25, 2021
MarijnS95 added a commit to MarijnS95/gtk-rs-core that referenced this pull request Aug 29, 2021
MarijnS95 added a commit to MarijnS95/gtk3-rs that referenced this pull request Aug 29, 2021
`impl` is quite a bit more concise and readable when a type parameter
with trait bound is only used once.  Aside that, this simplifies some of
the type handling in trampolines by using `P: IsA<>` directly instead of
through a `where P: ...` indirection.
MarijnS95 added a commit to MarijnS95/gtk-rs-core that referenced this pull request Sep 3, 2021
MarijnS95 added a commit to MarijnS95/gtk3-rs that referenced this pull request Sep 3, 2021
MarijnS95 added a commit to MarijnS95/gtk-rs-core that referenced this pull request Sep 3, 2021
@sdroege
Copy link
Member

sdroege commented Sep 20, 2021

@GuillaumeGomez Should we get this in now then?

@GuillaumeGomez
Copy link
Member

This is good enough for now. Thanks @MarijnS95 !

@GuillaumeGomez GuillaumeGomez merged commit ede0c31 into gtk-rs:master Sep 20, 2021
@sdroege
Copy link
Member

sdroege commented Sep 20, 2021

@MarijnS95 You update your gtk-rs-core/gtk3-rs/gtk4-rs/gstreamer-rs PRs? :)

@MarijnS95 MarijnS95 deleted the bounds-impl branch September 20, 2021 18:17
MarijnS95 added a commit to MarijnS95/gtk-rs-core that referenced this pull request Sep 20, 2021
MarijnS95 added a commit to MarijnS95/gtk3-rs that referenced this pull request Sep 20, 2021
@MarijnS95
Copy link
Contributor Author

MarijnS95 commented Sep 20, 2021

@sdroege Updated everything except the gtk4-rs PR, that one should be reopened before I can force-push (otherwise GH looses the PR<->branch relationship) (Bilel just reopened as I said this) :)

MarijnS95 added a commit to MarijnS95/gtk4-rs that referenced this pull request Sep 20, 2021
gstreamer-github pushed a commit to sdroege/gstreamer-rs that referenced this pull request Sep 21, 2021
sdroege pushed a commit to MarijnS95/gtk4-rs that referenced this pull request Sep 22, 2021
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.

5 participants