gtk-rs / gir

Tool to generate rust bindings and user API for glib-based libraries
https://gtk-rs.org/gir/book/
MIT License
230 stars 102 forks source link

Autogenerate field accessors #935

Open russel opened 4 years ago

russel commented 4 years ago

From what I can tell (by applying gir to the GstMpegts-1.0.gst file), there is no auto-generation of safe accessors for the fields of the various structs. I think the GIR file itself does not mention accessors.

However I see that GtkD has autogeneration of accessors: gir-to-d seems to generate accessors for fields even if there is no specification of such a method in the GIR file.

If gir were to autogenerate accessors, it would save an awful lot of manual writing of said accessors.

sdroege commented 4 years ago

That would have to be opt-in per accessor at least as it's generally not clear if a field can be modified or not, and under which circumstances (might need a mutex, for example).

Generally with gobject-introspection it's discouraged to have API that requires direct field access.

russel commented 4 years ago

I was thinking of read only getters. I agree setters should be manually provided.

I am not sure that a data-oriented system can have anything other than a data-oriented API, so getters at least are needed. Clearly in an object-oriented system getters break encapsulation.

sdroege commented 4 years ago

Even for getters you have the problem that it's not clear what the access rules are. Sometimes you'd need to take a mutex first, sometimes it's actually private and you should not access it at all, ...

In D they probably went the C approach of making every hard problem the user's problem instead.

russel commented 4 years ago

From what I can see, GtkD generation does not try to solve the multithreading case as part of the generated binding. By leaving the threading as a problem for the layer above it can happily generate all the getters (and indeed it generates quite a lot of setters). In effect it is just creating a D binding to the C API. By introducing multi-threading and issues of privacy, or indeed relevance, of access, the Rust process is no longer just creating a binding, but is creating a new API that just happens to make use of the C API. Should it be an option to gir that would then enable both approaches to working with a C API at the choice of the people working on the Rust binding?

sdroege commented 4 years ago

What you're looking for is the -sys mode then, and that already has accessors for all the struct fields. And as the "access mode" is not clear, this requires unsafe code for using it.

russel commented 4 years ago

I am not sure that is true. On further reflection, I'd say the GtkD level is above the Rust -sys layer, but lower than the higher Rust layer you are saying should apply. For getters of atomic types such as u8, u16, etc. the issue is unsafe and yet it is safe to get them without worry. Autogeneration of getters for these is not a problem – the value added is removing the need for unsafe and use of low level names.

An aspect of the issue here is the gir autogeneration of Display trait implementations, which seems at the wrong level. For example:

impl fmt::Display for ContentNibbleHi {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        write!(f, "ContentNibbleHi::{}", match *self {
            ContentNibbleHi::MovieDrama => "MovieDrama",
            ContentNibbleHi::NewsCurrentAffairs => "NewsCurrentAffairs",
            ContentNibbleHi::ShowGameShow => "ShowGameShow",
            ContentNibbleHi::Sports => "Sports",
            ContentNibbleHi::ChildrenYouthProgram => "ChildrenYouthProgram",
            ContentNibbleHi::MusicBalletDance => "MusicBalletDance",
            ContentNibbleHi::ArtsCulture => "ArtsCulture",
            ContentNibbleHi::SocialPoliticalEconomics => "SocialPoliticalEconomics",
            ContentNibbleHi::EducationScienceFactual => "EducationScienceFactual",
            ContentNibbleHi::LeisureHobbies => "LeisureHobbies",
            ContentNibbleHi::SpecialCharacteristics => "SpecialCharacteristics",
            _ => "Unknown",
        })
    }
}

I currently see no reason for these being generated, they present strings at a low level (creating strings that are effectively Debug strings, and easily creatable using the Debug trait implementation) and their existence in generated code stops implementation of these traits in hand written code for the types focussed on higher-level strings.

sdroege commented 4 years ago

That's why I don't auto-generate any Display impls in the GStreamer bindings, I completely agree with that part.