Closed i509VCB closed 2 years ago
Looks like the proper fix is going to be pending future rust releases, per
error[E0119]: conflicting implementations of trait `std::borrow::ToOwned` for type `events::Event<&std::ffi::OsStr>`
--> src/events.rs:218:1
|
218 | impl<'a> ToOwned for Event<&'a OsStr> {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: conflicting implementation in crate `alloc`:
- impl<T> ToOwned for T
where T: Clone;error[E0119]: conflicting implementations of trait `std::borrow::ToOwned` for type `events::Event<&std::ffi::OsStr>`
--> src/events.rs:218:1
|
218 | impl<'a> ToOwned for Event<&'a OsStr> {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: conflicting implementation in crate `alloc`:
- impl<T> ToOwned for T
where T: Clone;
For now I will make into_owned available outside of the stream feature then.
Hi @i509VCB, thank you for your pull requests! I'm currently on vacation, but I'll be back next week and will take a look then (or maybe the week after, if there's too much of a backlog to catch up on).
Ah no worries
I was curious about this, and from this StackOverflow question it sounds like it is not really needed to implement the ToOwned
trait unless there is a specific use case that needs that trait in particular. Having the Event.to_owned()
non-trait method may be sufficient, if I understand correctly.
I am not familiar with the calloop crate, but a search over its GitHub source does not find any references to the ToOwned
trait.
So in the case of calloop, I need to have an owned event since an event source in calloop has an Event assoicated type. If the assoicated type has a lifetime, then it gets quite ugly fast.
The Event.to_owned()
method already satisfies that need to get an owned Event, right? I'm having trouble understanding why the ToOwned
trait should be implemented in addition to that?
Reading the SO question again, it sounds like ToOwned
isn't really a fit for Event
, as its intent is to be applied to reference types like str
and OsStr
to get a String and OsString. Event
on the other hand is not a reference type. We can still define Event.to_owned()
as a convention as a lightweight clone, but that doesn't imply we must implement ToOwned
.
Maybe the proper solution here is to implement Clone ourselves (just rename to_owned()
to clone()
essentially), and then we can use the built-in ToOwned impl for Clone types. And then delete our to_owned()
method as it would delegate to clone()
.
Edit: That last paragraph didn't make sense as to_owned()
is converting the &OsStr
to an owned OsString
.
Okay, I now understand what was meant by this change "pending future Rust releases," presumably referring to impl specialization and RFC 1210. So hypothetically if/when RFC 1210 stabilizes then it will be possible to implement ToOwned
here.
Thank you for looking into this, @talklittle! I've looked at the documentation of ToOwned
, and I believe you're right: the trait doesn't fit Event
, and the existing to_owned
method should suffice.
Closing this issue. @i509VCB, feel free to reopen, if you disagree.
I am using inotify with calloop and one issue I've noticed is the lack of ability to easily convert an event from the methods into an owned event to pass to calloop. Calloop gets kinda messy when an
Event
in an event source has a lifetime, so getting an owned copy is typically the best way around this issue.I noticed the presence of
into_owned
that is only available with the streams api, I think we could convert this to delegate to an implementation ofToOwned
and made always available.