Use &[impl AsRef<str>] instead of &[&str]

[rafaelcaricio]

I've skip all code under auto folders as those are auto-generated. Maybe I could update the automatic code generation as well? 🤔

[bilelmoussaoui]

I've skip all code under auto folders as those are auto-generated. Maybe I could update the automatic code generation as well? 🤔

Yes that would require some changes in gir (the utility used for generating the auto code).

Do you want to have a look at updating that one as well?

[rafaelcaricio]

@bilelmoussaoui Yes, I just started browsing the gir codebase 🔍

[bilelmoussaoui]

I still see various : &str in glib/src/object.rs from a quick search https://github.com/gtk-rs/gtk-rs-core/blob/master/glib/src/object.rs#L1259-L1277 there are probably others btw :)

[sdroege]

Let's focus only on [&str] in this PR and keep plain &str for a later PR with separate discussion. The advantage seems less big there to me.

Also this needs some changes in https://github.com/gtk-rs/gir :)

Thanks for looking into this, @rafaelcaricio!

[rafaelcaricio]

I've opened a PR in the gir repo to complement the changes: gtk-rs/gir#1149

[bilelmoussaoui]

I've opened a PR in the gir repo to complement the changes: gtk-rs/gir#1149

Do you mind using that branch of gir and regenerate gtk-rs-core so we can see how those changes looks like?

[rafaelcaricio]

@bilelmoussaoui Would you like me to commit the generated changes here?

[bilelmoussaoui]

@bilelmoussaoui Would you like me to commit the generated changes here?

yes please

[MarijnS95]

Cross-posting gtk-rs/gir#1149 (comment), perhaps &[impl AsRef<str>] might work to save on the extra trait bounds that otherwise have to be added.

[rafaelcaricio]

@MarijnS95 Right, that would simplify things indeed. Just doesn't look natural!? Happy to make the change anyways.. just let me know what you all prefer.

[MarijnS95]

@MarijnS95 Right, that would simplify things indeed. Just doesn't look natural!? Happy to make the change anyways.. just let me know what you all prefer.

I personally prefer it since it's more concise, but this is not my project and I'm not a maintainer so :)

[sdroege]

I'd prefer impl trait if we don't ever need to use the type variable inside the code.

[MarijnS95]

I wonder if we could reduce trait bound complexity even further by using impl Trait in more places. But that's definitely a thing for a separate commit/PR!

[sdroege]

I'm all for it, please go ahead if you have some time :)

[MarijnS95]

I'm all for it, please go ahead if you have some time :)

Easy enough I'd say, gtk-rs/gir#1153.

Still needs some more testing and scrutineering, but it's time for breakfast first :P

[rafaelcaricio]

If I understand the discussion here correctly, the PR gtk-rs/gir#1153 supersedes my PR gtk-rs/gir#1149 ? Now I should probably revert the commit I have in this PR to remove the generated code from my PR to gir?

What would be the next steps? I'm a bit confused now.

[MarijnS95]

If I understand the discussion here correctly, the PR gtk-rs/gir#1153 supersedes my PR gtk-rs/gir#1149 ? Now I should probably revert the commit I have in this PR to remove the generated code from my PR to gir?

What would be the next steps? I'm a bit confused now.

Hey!

Apologies for the confusion!

Your PR is not superseded at all :). Ideally we get gtk-rs/gir#1153 in first, as that only switches to impl, and then get gtk-rs/gir#1149 plus this PR on top to use AsRef<str> strings too!

I still have some finishing up to do, GStreamer-rs is pretty lenient but gtk-rs uses all the special types (async and futures with special trait bounds). After that I can help rebase your gir PR on top unless you wan to take care of that yourself.

Let me know what approach you prefer! We can always get your gir PR merged first, and that I rebase my work on top (though that PR is already rather loaded and substantial, it's much easier to rebase smaller, trivial changes like yours on top instead of the other way around).

[rafaelcaricio]

@MarijnS95 I'm fine with waiting for your PR to get in first. This is my first contribution to those projects so I'm still figuring out my way around the code.

[rafaelcaricio]

On the case of using impl AsRef<str>, the current version of gtk-rs/gir#1149 is generic enough to not break on callbacks that need to use the bounds. But I guess the gtk-rs/gir#1153 will help with differentiating the usages.

[rafaelcaricio]

I've updated the PR with my latest changes, now we use &[impl AsRef<str>]. I still need to update the manual code, but that is easy. :)

[MarijnS95]

I've updated the PR with my latest changes, now we use &[impl AsRef<str>]. I still need to update the manual code, but that is easy. :)

It doesn't look like all occurences were replaced, some are still using an explicit type parameter. Was that intended?

[rafaelcaricio]

@MarijnS95 Could you point to one example where it was not replaced?

[MarijnS95]

Quite a few places 😉

[rafaelcaricio]

@MarijnS95 those are all manually added. That's why I wrote "I still need to update the manual code, but that is easy." :) Those occurrences are not generated by gir.

[MarijnS95]

@MarijnS95 those are all manually added. That's why I wrote "I still need to update the manual code, but that is easy." :) Those occurrences are not generated by gir.

Gotcha, I think I misread your original comment, but you definitely said that :)

I cannot mark the comments as resolved though, not enough rights here :/

[rafaelcaricio]

the rest of the code fails to compile with a missing trait bound for to_glib_none() on &[impl AsRef<str>].

This is where I'm stuck. I cannot find a way to implement that trait bound without conflicting with existing trait bounds. 😢

[rafaelcaricio]

Not sure this will actually compile, maybe after the trait bounds validation the compiler might complain about this ownership. But I can work out a solution for that later, that is fine..

[MarijnS95]

This is where I'm stuck. I cannot find a way to implement that trait bound without conflicting with existing trait bounds.

It's a really tricky bugger indeed. I tried the obvious in implementing (with todo!(), no time to waste on the actual code just yet):

impl<'a, S: AsRef<str> + ToGlibPtr<'a, *const c_char>>
    ToGlibContainerFromSlice<'a, *const *const c_char> for S

(And later the various mut variants)

And that conflicts with the existing implementations. Neat I'd say, those should all be able to go through AsRef as a single shared implementation... Comment them out but noupe, now other traits are missing :/

[sdroege]

It's a really tricky bugger indeed.

The alternative would be to go with let x = x.iter().map(|i| i.as_ref()).collect::<Vec<_>>() as a first step. All the strings have to be reallocated and copied to a new vec anyway for the NUL-termination, doing another shallow copy of the slice is not going to make things noticeably worse :)

[MarijnS95]

The alternative would be to go with let x = x.iter().map(|i| i.as_ref()).collect::<Vec<_>>() as a first step. All the strings have to be reallocated and copied to a new vec anyway for the NUL-termination, doing another shallow copy of the slice is not going to make things noticeably worse :)

We'd have to insert this for autogenerated code too, but I think the Transformation system already lets you specify that with ease?

Alternatively Bound has its own extra_to_glib (or something like that). &x.iter().map(|i| i.as_ref()).collect::<Vec<_>>() should work though as the temporary is only dropped after the function is called.

[rafaelcaricio]

@MarijnS95 Yes, I tried multiple ways... nothing worked, it always conflicted with something else.

The suggestion from @sdroege was what I also think would be the best option. Any pointers to where to add that in gir are appreciated. 😄

[sdroege]

This would rather use impl IntoStrV now, and that's tracked by another issue already