Skip to content
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

Various changes to how we automatically derive trait impls for boxed/shared types #367

Merged
merged 16 commits into from Aug 17, 2018

Conversation

Projects
None yet
3 participants
@sdroege
Copy link
Member

sdroege commented Aug 2, 2018

This is still WIP and requires changes to gir that I'm currently working on.

Comments about various parts below

This is to solve #247 and generally improve our deriving story.

@@ -52,7 +53,7 @@ macro_rules! glib_boxed_wrapper {
([$($attr:meta)*] $name:ident, $ffi_name:path, @copy $copy_arg:ident $copy_expr:expr,
@free $free_arg:ident $free_expr:expr) => {
$(#[$attr])*
#[derive(Clone, Debug)]

This comment has been minimized.

@sdroege

sdroege Aug 2, 2018

Author Member

The idea here is that for shared and boxed types, the user of the macro adds #[derive(Debug, Hash, PartialEq, Eq)] or a subset of those as needed and implements the other ones.

gir does this automatically (Debug only) unless the derives configuration is given. Then it only derives those given in there

This comment has been minimized.

@sdroege

sdroege Aug 2, 2018

Author Member

Also gir will not derive trait impls if there is a specific function for them available

// Always derive Debug/Hash (and below impl PartialEq, Eq) for object types. Due to
// inheritance and up/downcasting we must implement these by pointer or otherwise
// they would potentially give differeny results for the same object depending on
// the type we currently know for it

This comment has been minimized.

@sdroege

sdroege Aug 2, 2018

Author Member

This one here is quite important. No behaviour change but an explanation.

Some changes in gir necessary to actually expose any specific functions for those so that they are still available. For some it does, for some it doesn't yet.

I also have gir changes to implement Hash if possible with a specific function

This comment has been minimized.

@sdroege

sdroege Aug 6, 2018

Author Member

Actually the same applies for Ord and PartialOrd. I think we should move those out of here too (I don't think there currently is any impl anyway).

In the future we might want to consider adding wrapper types for those that deref to the actual type and can only hold that type

This comment has been minimized.

@sdroege

sdroege Aug 10, 2018

Author Member

It generally does not make sense to implement these traits in a non-generic way. Hash, Eq, PartialOrd and Ord are all constrained by our generic PartialEq implementation that simply compares pointers and thus must all be equally generic (same pointer must give equality).

Display is an interesting case that might make sense to be implemented around to_string() functions but the problem here is that we would only implement it for the specific type and not any of its subtypes. It is more ergonomic to not implement Display but to expose as to_string() as that way it can be directly called on any subclass.

In summary, I would propose to not implement any of these traits on any objects in a non-generic way and simply expose the functions as in C.

This comment has been minimized.

@EPashkin

EPashkin Aug 10, 2018

Member

Agree that it good reason not implement most trait, but not sure about Hash

This comment has been minimized.

@EPashkin

EPashkin Aug 10, 2018

Member

Also IMHO we need add function for compare two IsA<Object> by compare pointers.

This comment has been minimized.

@sdroege

sdroege Aug 10, 2018

Author Member

Agree that it good reason not implement most trait, but not sure about Hash

There's a section about Hash and its relation to Eq in the docs. Equal things need to have the same hash, which is obviously the case if the pointers are the same. But we would also have the same hash with different pointers and same "content" which is far from ideal, hash collisions :)

Also IMHO we need add function for compare two IsA by compare pointers.

You mean Ord? Because Eq we have

This comment has been minimized.

@EPashkin

EPashkin Aug 10, 2018

Member

I meant Eq good it we have it already. I don't see it.

This comment has been minimized.

@sdroege

sdroege Aug 10, 2018

Author Member

glib/src/object.rs

Lines 566 to 572 in 7d9217b

impl<T: $crate::object::IsA<$crate::object::Object>> ::std::cmp::PartialEq<T> for $name {
#[inline]
fn eq(&self, other: &T) -> bool {
use $crate::translate::ToGlibPtr;
self.0.to_glib_none().0 == other.to_glib_none().0
}
}
and
impl ::std::cmp::Eq for $name { }

I also added similar things for Ord in these commits now

@sdroege

This comment has been minimized.

Copy link
Member Author

sdroege commented Aug 10, 2018

Some more commits here. I'm not sure what to do about Ord/PartialOrd for Boxed/Shared types. They're useful to have (per pointer) for BTreeMap for example so I guess it would make sense to add the default impls of them to the boxed types that don't have a better impl

On top of these commits I'll also need to do further changes to gir, those come next week I guess

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Aug 10, 2018

@sdroege Looks good.
I don't understand question about 'Ord' for Boxed/Shared:
you already implement it for base type, so user can derive or implement it yourself.

@sdroege

This comment has been minimized.

Copy link
Member Author

sdroege commented Aug 11, 2018

If we should derive it for our boxed types by pointer, and generate it by default. Same for enums and flags types :) what do you think? Main use case is BTreeMap like data structures

@sdroege

This comment has been minimized.

Copy link
Member Author

sdroege commented Aug 13, 2018

@GuillaumeGomez @EPashkin Any opinions on liberally adding PartialOrd / Ord impls on everything for making BTreeMap-like data structures usable?

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Aug 13, 2018

I think I already gave my opinion on this but if it can make the situation better then we should definitely do it.

@sdroege

This comment has been minimized.

Copy link
Member Author

sdroege commented Aug 13, 2018

Ok, I'll go for that then. Impls for our types, automatically and for all enums/flags too. And on objects we only have generic by-pointer impls of everything due to subclassing weirdnesses.

@sdroege sdroege force-pushed the sdroege:derive branch from a714558 to aaf1430 Aug 17, 2018

@sdroege sdroege force-pushed the sdroege:derive branch from aaf1430 to feff6a6 Aug 17, 2018

@sdroege

This comment has been minimized.

Copy link
Member Author

sdroege commented Aug 17, 2018

This should be all good now, and should be merged together with gtk-rs/gir#627 and then regens everywhere

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Aug 17, 2018

Thanks!

@GuillaumeGomez GuillaumeGomez merged commit 4cbcd91 into gtk-rs:master Aug 17, 2018

0 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
@sdroege

This comment has been minimized.

Copy link
Member Author

sdroege commented Aug 17, 2018

Thanks, I'll send regens in an hour or two when I'm back home

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.