Union, Clone and Copy #527

ghost · 2018-01-29T16:01:48Z

Functionally wise this PR introduces support for unions and derives Clone and
Copy traits for ffi types (based on @Luke-Nukem work from #415).

It also temporarily removes support for structs with partially generated
fields. In particular types with bitfields are not generated, with exception of
GdkEventKey and GdkEventScroll. Exception is made because there is manual code
that uses those.

From implementation perspective this change tries to migrate most logic to
phases that occur before code generation, so that codegen is only concerned
with joining some string together, so to say. At this point this may not be
readily visible, as codegen cleanup will follow in a later PR.

Tested with all crates hosted under gtk-rs (regenerated ffi and api crates).
soup-sys-rs requires manual tweak to import sockaddr from libc.

This PR incorporates both #415 and #526.

General idea is to ensure that once postprocessing phase is completed, the types themselves describe exactly which types will be generated. This would allow to make sys code generation to be completely unconditional, but this still remains to be done.

There is manually written code that uses their fields.

EPashkin · 2018-01-29T16:28:21Z

Seems blocked by gtk-rs/gir-files#13 as attempt to generate pangocairo causes "thread 'main' has overflowed its stack" runtime error

ghost · 2018-01-29T16:33:31Z

@EPashkin thanks for pointing that out. I am running this with PangoCairo fixed
as descried in gtk-rs/gir-files#13, i.e., all fields are removed from FcFontMap.

EPashkin · 2018-01-29T16:42:08Z

Generate empty lines in gdk-sys

@@ -3210,6 +3229,7 @@ impl ::std::fmt::Debug for GdkEventGrabBroken {
 }
 
 #[repr(C)]
+
 pub struct GdkEventKey {
     pub type_: GdkEventType,
     pub window: *mut GdkWindow,
@@ -3354,6 +3381,7 @@ impl ::std::fmt::Debug for GdkEventProximity {
 }
 
 #[repr(C)]
+
 pub struct GdkEventScroll {
     pub type_: GdkEventType,
     pub window: *mut GdkWindow,

Examples builds fine but there warning about impl ::std::fmt::Debug for GDoubleIEEE754 { and impl ::std::fmt::Debug for GFloatIEEE754

    --> d:/eap/rust/0/sys/glib-sys\src\lib.rs:1121:9
     |
1121 |         unsafe {
     |         ^^^^^^ unnecessary `unsafe` block
     |
     = note: #[warn(unused_unsafe)] on by default

warning: unnecessary `unsafe` block
    --> d:/eap/rust/0/sys/glib-sys\src\lib.rs:1133:9
     |
1133 |         unsafe {
     |         ^^^^^^ unnecessary `unsafe` block

and impl ::std::fmt::Debug for GdkEvent

warning: unnecessary `unsafe` block
    --> d:/eap/rust/0/sys/gdk-sys\src\lib.rs:3028:9
     |
3028 |         unsafe {
     |         ^^^^^^ unnecessary `unsafe` block
     |
     = note: #[warn(unused_unsafe)] on by default

ghost · 2018-01-30T19:54:55Z

I pushed commit with codegen cleanup mentioned earlier.
The support for truncated struct types is now back as well.

EPashkin · 2018-01-30T20:22:07Z

@tmiasko Thanks, I check it tomorrow.
Currently I only can say that it add too many unneeded empty lines

EPashkin · 2018-01-30T20:24:17Z

src/codegen/sys/fields.rs

+
+fn analyze_fields(env: &Env, unsafe_access: bool, fields: &[Field]) -> (Vec<FieldInfo>, Option<String>) {
+    let mut truncated = None;
+    let mut infos = Vec::new();


Maybe use Vec::with_capacity(fields.len) ?

* Reuse is incomplete check from analysis phase. The Library::fix_field establishes all invariants required for analysis::types::IsIncomplete, so we can just use it in Library::make_unrepresentable_types_opaque. As a minor drawback we no longer have information why type is considered incomplete. * Preallocate memory for fields in analyze_fields. * Fix condition checking number of parameters for which Rust still derives traits. It should be inclusive. * Remove unnecessary whitespace in sys code generation.

EPashkin · 2018-01-31T03:24:00Z

src/codegen/sys/fields.rs

+
+fn field_ffi_type(env: &Env, field: &Field) -> Result {
+    if field.is_incomplete(&env.library) {
+        return Err(TypeError::Ignored(format!("field {} has incomplete type", &field.name)));


IMHO better also show type here (not insist, as do it troublesome at first glance).

#[repr(C)] pub struct GtkAboutDialogClass { _truncated_record_marker: c_void, // /*Ignored*/field parent_class has incomplete type }

It will be serious problem to show right type, as bit field also reason for incompleteness
and /*Ignored*/field null_fold_if_empty has incomplete type u32 looks strange.
So ignore this proposal

EPashkin · 2018-01-31T03:53:49Z

src/analysis/types.rs

+impl IsPtr for Field {
+    fn is_ptr(&self) -> bool {
+        if let Some(ref c_type) = self.c_type {
+            return c_type.contains("*")


Clippy nags about this and others contains("*"):

warning: single-character string constant used as pattern --> src\analysis\types.rs:16:36 | 16 | return c_type.contains("*") | ----------------^^^- help: try using a char instead: `c_type.contains('*')` | = note: #[warn(single_char_pattern)] on by default = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.183/index.html#single_char_pattern

EPashkin · 2018-01-31T03:57:45Z

src/library_postprocessing.rs

+                                continue;
+                            }
+                            let field_type = self.type_(field.typ);
+                            if let Some(_) = field_type.maybe_ref_as::<Function>() {


warning: redundant pattern matching, consider using `is_some()` --> src\library_postprocessing.rs:303:36 | 303 | if let Some(_) = field_type.maybe_ref_as::<Function>() { | -------^^^^^^^---------------------------------------- help: try this: `if field_type.maybe_ref_as::<Function>().is_some()` | = note: #[warn(if_let_redundant_pattern_matching)] on by default = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.183/index.html#if_let_redundant_pattern_matching

EPashkin · 2018-01-31T04:03:41Z

src/library_postprocessing.rs

+                    Type::Union(Union { ref name, ref fields, ..}) => {
+                        for (fid, field) in fields.iter().enumerate() {
+                            if nameutil::needs_mangling(&field.name) {
+                                let new_name = nameutil::mangle_keywords(&*field.name).to_string();


Maybe .into_owned() instead?

EPashkin · 2018-01-31T04:06:56Z

src/library_postprocessing.rs

}
+

Unneeded line

EPashkin · 2018-01-31T04:07:45Z

src/parser.rs

-                            {
-                                Type::union(self, u, ns_id)
-                            }
+                            Type::union(self, u, ns_id)
                        };


Just let type_id = Type::union(self, u, ns_id) ?

Or move into fields.push

EPashkin · 2018-01-31T04:08:07Z

src/parser.rs

-                                {
-                                    Type::union(self, u, ns_id)
-                                }
+                                Type::union(self, u, ns_id)


EPashkin · 2018-01-31T04:10:08Z

src/parser.rs

@@ -591,7 +583,7 @@ impl Library {
                                        s.push('_');
                                        s
                                    })
-                                    .unwrap_or_else(String::new),
+                                    .unwrap_or("".into()),


Please change it back, and next too

.. as clippy nags

EPashkin · 2018-01-31T04:18:20Z

src/codegen/sys/statics.rs

@@ -52,7 +47,7 @@ pub fn only_for_glib(w: &mut Write) -> Result<()> {
        "pub type gpointer = *mut c_void;",
        "",
        "#[repr(C)]",
-        "#[derive(Debug)]",
+        "#[derive(Copy,Clone,Debug)]",


#[derive(Copy, Clone, Debug)] ?

EPashkin · 2018-01-31T04:30:10Z

src/analysis/types.rs

+        }
+    }
+}
+


Unneeded line

This still actual

EPashkin · 2018-01-31T04:44:13Z

@tmiasko, @sdroege, @GuillaumeGomez There will be no problem with union Debug implementation?
I worry about cases when value unrepresentative on some fields, but not sure that it can be happened.

#[repr(C)]
#[derive(Copy, Clone)]
pub union GtkBindingArg_d {
    pub long_data: c_long,
    pub double_data: c_double,
    pub string_data: *mut c_char,
}

impl ::std::fmt::Debug for GtkBindingArg_d {
    fn fmt(&self, f: &mut ::std::fmt::Formatter) -> ::std::fmt::Result {
        f.debug_struct(&format!("GtkBindingArg_d @ {:?}", self as *const _))
         .field("long_data", unsafe { &self.long_data })
         .field("double_data", unsafe { &self.double_data })
         .field("string_data", unsafe { &self.string_data })
         .finish()
    }
}

EPashkin · 2018-01-31T04:48:35Z

Also we need print padding values? (There can be needed "padding" fields)

#[repr(C)]
#[derive(Copy, Clone)]
pub union GtkTextAttributes_u1 {
    pub font_features: *mut c_char,
    pub padding: [c_uint; 2],
}

impl ::std::fmt::Debug for GtkTextAttributes_u1 {
    fn fmt(&self, f: &mut ::std::fmt::Formatter) -> ::std::fmt::Result {
        f.debug_struct(&format!("GtkTextAttributes_u1 @ {:?}", self as *const _))
         .field("font_features", unsafe { &self.font_features })
         .field("padding", unsafe { &self.padding })
         .finish()
    }
}

EPashkin · 2018-01-31T04:52:29Z

Note: almost all GUI "xxClass" now incomplete due bit field in GtkContainerClass

@@ -3083,20 +3494,24 @@ pub struct GtkContainerClass {
     pub get_child_property: Option<unsafe extern "C" fn(*mut GtkContainer, *mut GtkWidget, c_uint, *mut gobject::GValue, *mut gobject::GParamSpec)>,
     pub get_path_for_child: Option<unsafe extern "C" fn(*mut GtkContainer, *mut GtkWidget) -> *mut GtkWidgetPath>,
     _truncated_record_marker: c_void,
-    //_handle_border_width: unsigned: 1,
-    //_gtk_reserved1: fn,
-    //_gtk_reserved2: fn,
-    //_gtk_reserved3: fn,
-    //_gtk_reserved4: fn,
-    //_gtk_reserved5: fn,
-    //_gtk_reserved6: fn,
-    //_gtk_reserved7: fn,
-    //_gtk_reserved8: fn,
+    // /*Ignored*/field _handle_border_width has incomplete type
 }

ghost · 2018-01-31T06:39:00Z

My thoughts about issues raised by @EPashkin, and some of my own:

Should we ignore Debug impl for Unions? I start with assumption that Rust
treats unions essentially as sequence of bytes (which is somewhat open
question at the moment AFAIK). On that basis, it would seem that accessing
union fields should be fine. We only have primitive types there (or other
struct / union that have only primitive types), and pointers are NOT
dereferenced.
Should we ignore private fields in Debug? The padding fields should fall into
this category. I think we should omit them from Debug impl and will update
code latter accordingly.
Should we generate private fields with private visibility, i.e., without "pub"?
I think we should, but glib crate currently accesses those ...

Thanks for review @EPashkin.

ghost · 2018-01-31T07:30:42Z

I pushed two commits. First addresses @EPashkin review comments.
Second one omits private and volatile fields from Debug implementation.

It did help to some degree with printing padding fields,
but some are still left like in GSettingsClass.

GuillaumeGomez · 2018-01-31T08:56:19Z

The best way to check if Debug implementations are working is to actually test them. Like that, we'll know.

EPashkin · 2018-01-31T15:56:19Z

@GuillaumeGomez I forgot that we can test it ;)

let u = gtk_ffi::GtkBindingArg_d { long_data: 0x12345678i32 };
println!("{:?}", u);

print

GtkBindingArg_d @ 0x58df2d8 { long_data: 305419896, double_data: 0.000000000000000000000000000000000000000000000000000000000000000000000000000
0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000002272893269, string_data: 0x112345678 }

EPashkin · 2018-01-31T16:00:51Z

src/codegen/sys/statics.rs

@@ -52,7 +47,7 @@ pub fn only_for_glib(w: &mut Write) -> Result<()> {
        "pub type gpointer = *mut c_void;",
        "",
        "#[repr(C)]",
-        "#[derive(Debug)]",
+        "#[derive(Copy,Clone)]",


"#[derive(Copy, Clone)]", ?

Union containing volatile field need to be copyable as well.
Alternatively we could stop generating unions with volatile fields.

Sorry that it was unclear: You just forgot space between.

Please make 2 last fixes change current commits.

EPashkin · 2018-01-31T16:10:10Z

Changes from last 2 commits:

@@ -1261,11 +1258,9 @@ pub struct GApplicationCommandLineClass {
 impl ::std::fmt::Debug for GApplicationCommandLineClass {
     fn fmt(&self, f: &mut ::std::fmt::Formatter) -> ::std::fmt::Result {
         f.debug_struct(&format!("GApplicationCommandLineClass @ {:?}", self as *const _))
-         .field("parent_class", &self.parent_class)
          .field("print_literal", &self.print_literal)
          .field("printerr_literal", &self.printerr_literal)
          .field("get_stdin", &self.get_stdin)
-         .field("padding", &self.padding)
          .finish()
     }
 }
@@ -1554,7 +1549,6 @@ pub struct GDBusAnnotationInfo {
 impl ::std::fmt::Debug for GDBusAnnotationInfo {
     fn fmt(&self, f: &mut ::std::fmt::Formatter) -> ::std::fmt::Result {
         f.debug_struct(&format!("GDBusAnnotationInfo @ {:?}", self as *const _))
-         .field("ref_count", &self.ref_count)
          .field("key", &self.key)
          .field("value", &self.value)
          .field("annotations", &self.annotations)
@@ -4168,8 +4145,6 @@ pub struct GSimpleActionGroupClass {
 impl ::std::fmt::Debug for GSimpleActionGroupClass {
     fn fmt(&self, f: &mut ::std::fmt::Formatter) -> ::std::fmt::Result {
         f.debug_struct(&format!("GSimpleActionGroupClass @ {:?}", self as *const _))
-         .field("parent_class", &self.parent_class)
-         .field("padding", &self.padding)
          .finish()
     }
 }
@@ -1144,8 +1144,6 @@ pub union GMutex {
 impl ::std::fmt::Debug for GMutex {
     fn fmt(&self, f: &mut ::std::fmt::Formatter) -> ::std::fmt::Result {
         f.debug_struct(&format!("GMutex @ {:?}", self as *const _))
-         .field("p", unsafe { &self.p })
-         .field("i", unsafe { &self.i })
          .finish()
     }
 }
@@ -833,15 +830,6 @@ pub struct PangoFontMetrics {
 impl ::std::fmt::Debug for PangoFontMetrics {
     fn fmt(&self, f: &mut ::std::fmt::Formatter) -> ::std::fmt::Result {
         f.debug_struct(&format!("PangoFontMetrics @ {:?}", self as *const _))
-         .field("ref_count", &self.ref_count)
-         .field("ascent", &self.ascent)
-         .field("descent", &self.descent)
-         .field("approximate_char_width", &self.approximate_char_width)
-         .field("approximate_digit_width", &self.approximate_digit_width)
-         .field("underline_position", &self.underline_position)
-         .field("underline_thickness", &self.underline_thickness)
-         .field("strikethrough_position", &self.strikethrough_position)
-         .field("strikethrough_thickness", &self.strikethrough_thickness)
          .finish()
     }
 }

ghost · 2018-01-31T17:26:41Z

Done. I also included hack for GHookList so that gstreamer bindings remain more or less working.

EPashkin · 2018-01-31T17:43:41Z

Thanks

EPashkin · 2018-02-01T15:56:28Z

Ok, no one against PR, so I will merge it.

Union, Clone and Copy

Luke Jones and others added 12 commits November 13, 2017 10:19

Unions: Remove feature-gate for stable rustc

21a7e4f

Unions: make all structs and unions copy,clone

a5e2bbb

Unions: tweaks to copy,clone, and remove unneeded if/else

aeefc4d

unions: rebase & fix copy,clone for unions

8560db7

tests: update some crate versions, eg, bitflags

0e6665f

Merge branch 'master' into analysis.

91bad85

Merge remote-tracking branch 'luke-nukem/master' into analysis

4348f78

Remove unused code.

a4a4f21

Comment derive clause if necessary.

b69e1f4

Introduce back non-representable structures as struct name(c_void).

93925a4

Support for GdkEventKey and GdkEventScroll.

edc3a95

There is manually written code that uses their fields.

tmiasko added 2 commits January 30, 2018 20:25

Refactored union, struct and class sys code generation.

438d3ec

Restore support for truncated types.

f9de6fd

EPashkin reviewed Jan 30, 2018

View reviewed changes

EPashkin reviewed Jan 31, 2018

View reviewed changes

src/library_postprocessing.rs Outdated

}

Copy link

Member

EPashkin Jan 31, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unneeded line

EPashkin reviewed Jan 31, 2018

View reviewed changes

src/parser.rs Outdated

{

Type::union(self, u, ns_id)

}

Type::union(self, u, ns_id)

Copy link

Member

EPashkin Jan 31, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same

EPashkin reviewed Jan 31, 2018

View reviewed changes

tmiasko added 3 commits January 31, 2018 18:20

Address code review comments.

fc944fd

Omit private and volatile fields from Debug implementation.

980eefe

Necessary evil to support GHookList.

4635ba7

EPashkin merged commit f8eb742 into gtk-rs:master Feb 1, 2018

This was referenced Feb 1, 2018

Generate unions gtk-rs/sys#78

Merged

Generate unions in sourceview-sys gtk-rs/sourceview#40

Merged

ghost mentioned this pull request Feb 1, 2018

Don't directly print arrays > 32 elements in Debug impls in -sys crates #516

Closed

vhdirk pushed a commit to vhdirk/gir that referenced this pull request Jul 6, 2018

Merge pull request gtk-rs#527 from tmiasko/analysis

c2e46c9

Union, Clone and Copy

Union, Clone and Copy #527

Union, Clone and Copy #527

Conversation

ghost commented Jan 29, 2018

EPashkin commented Jan 29, 2018

ghost commented Jan 29, 2018

EPashkin commented Jan 29, 2018

ghost commented Jan 30, 2018

EPashkin commented Jan 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EPashkin Jan 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EPashkin commented Jan 31, 2018

EPashkin commented Jan 31, 2018

EPashkin commented Jan 31, 2018

ghost commented Jan 31, 2018

ghost commented Jan 31, 2018

GuillaumeGomez commented Jan 31, 2018

EPashkin commented Jan 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EPashkin commented Jan 31, 2018 • edited Loading

ghost commented Jan 31, 2018

EPashkin commented Jan 31, 2018

EPashkin commented Feb 1, 2018

EPashkin Jan 31, 2018 •

edited

Loading

EPashkin commented Jan 31, 2018 •

edited

Loading