-
Notifications
You must be signed in to change notification settings - Fork 567
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
impl Data
for a bunch of std types.
#1534
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. But I am not sure about the raw pointers.
druid/src/data.rs
Outdated
impl<T: 'static + ?Sized> Data for *const T { | ||
fn same(&self, other: &Self) -> bool { | ||
// This Does not dereference the pointer, only the reference to the pointer. | ||
ptr::eq(*self, *other) | ||
} | ||
} | ||
|
||
impl<T: 'static + ?Sized> Data for *mut T { | ||
fn same(&self, other: &Self) -> bool { | ||
// This Does not dereference the pointer, only the reference to the pointer. | ||
ptr::eq(*self, *other) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would leave these ones. When someone has raw pointers, they are better using same_fn
. But if you add these, then also add NonNull
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm OK I'm sold on removing the raw pointers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes I agree I struggle to think of a good case where you'd want this.
The user should handle this themselves safely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes sense, thanks!
I had a look at implementing it for |
This patch implements
Data
for some more types instd
. My motivation wasSystemTime
, but I thought I may as well implement it anywhere else it made sense.