Full enablement of Unions using nightly Rust #402

flukejones · 2017-07-06T01:13:25Z

Unions are fully enabled behind a feature-gate. Both non-nightly and nightly Rust appear to be working well.

EPashkin · 2017-07-06T06:00:01Z

@Luke-Nukem Can you rebase this to master with removing merges?
Why GVariantBuilder_u1 etc. added to all generated libs in https://github.com/gtk-rs/sys when this feature enabled?
Bad changes in glib-sys with bitsized fields: GDate as example

#[repr(C)]
 pub struct GDate {
-    _truncated_record_marker: c_void,
-    //julian_days: guint: 32,
-    //julian: guint: 1,
-    //dmy: guint: 1,
-    //day: guint: 6,
-    //month: guint: 4,
-    //year: guint: 16,
+    pub julian_days: c_uint,
+    pub julian: c_uint,
+    pub dmy: c_uint,
+    pub day: c_uint,
+    pub month: c_uint,
+    pub year: c_uint,
 }

Strange that GScanner has //union comments before, but now it seems fixed, thanks

EPashkin · 2017-07-06T06:14:12Z

src/parser.rs

+        // TODO: unsure of how to handle this
+        //if attrs.by_name("is-gtype-struct").is_some() {
+        //    return Ok(());
+        //}


Seems read_record return type need change to Result<Option<Type>>

Yes but it also needs an initialised Type to return. This is where I am unsure what to do... I could maybe return a default() initialised Type?

Why it can't return Ok(None) in this case? And don't add type in read_record_start

Result<Option<Type>>.... I don't know why my mind completely ignored that....
Done.

flukejones · 2017-07-06T07:15:55Z

Regarding the appending of _u1 to various unions or _s1 to nested union->structs, it's because without it we end up with each struct->union->struct having the same c_type, and thus the same name for each.

Regarding

#[repr(C)]
 pub struct GDate {
-    _truncated_record_marker: c_void,
-    //julian_days: guint: 32,
-    //julian: guint: 1,
-    //dmy: guint: 1,
-    //day: guint: 6,
-    //month: guint: 4,
-    //year: guint: 16,
+    pub julian_days: c_uint,
+    pub julian: c_uint,
+    pub dmy: c_uint,
+    pub day: c_uint,
+    pub month: c_uint,
+    pub year: c_uint,
 }

Well of course... We can't initialise a newly created struct like that. Have to declare it first, then initialise it. If it were an enum instead of a struct then it might work.

If you look at;

struct GDate {
  guint julian_days : 32; /* julian days representation - we use a
                           *  bitfield hoping that 64 bit platforms
                           *  will pack this whole struct in one big
                           *  int
                           */

  guint julian : 1;    /* julian is valid */
  guint dmy    : 1;    /* dmy is valid */

  /* DMY representation */
  guint day    : 6;
  guint month  : 4;
  guint year   : 16;
};

each field is a guint, which translates to a c_uint. And they are initialised (which we can't do in rust).

EPashkin · 2017-07-06T07:23:26Z

As I remember size GDate in C will be 64bit or 8bytes with julian, dmy and day stored in byte 5)
Your version: 6*4=24 bytes;

flukejones · 2017-07-06T09:39:36Z

As I remember size GDate in C will be 64bit or 8bytes with julian, dmy and day stored in byte 5)
Your version: 6*4=24 bytes;

Looking at guint it seems it will be a particular size depending on a #define G_MAXUINT UINT_MAX.

UINT_MAX is defined in Rust too, and c_uint size is defined by that. So essentially it looks like both guint and c_uint are defined the same way, and so should be the same size.

EPashkin · 2017-07-06T09:51:32Z

src/parser.rs

@@ -21,6 +21,7 @@ pub fn is_empty_c_type(c_type: &str) -> bool {
    c_type == EMPTY_CTYPE
 }

+


Unneeded extra line

EPashkin · 2017-07-06T09:54:55Z

src/parser.rs

+        let mut name = try!(attrs.by_name("name")
+                .ok_or_else(|| mk_error!("Missing record name", parser)));
+        let mut c_type = try!(attrs.by_name("type")
+                .ok_or_else(|| mk_error!("Missing c:type attribute", parser)));


rustfmt-nigthly change this 2 variables back.

Ah, my rustfmt is outdated. Are you running everything through rustfmt?

Not on regular basic, but sometimes.

Mostly because latest tend to split function declaration to multiple lines.

EPashkin · 2017-07-06T10:04:53Z

Seems you ignore that

guint julian : 1;
guint dmy : 1;

stored in one byte and all struct size can be lesser that guint size (not *2 as in your case).
Rust IMHO don't support this syntax,
this why we "ignore" stucts with these fields.
good that seems none of these structs used in unions,
so you can fallback to old code for this case

Also still not solved problem with including GVariantBuilder_u1 and GVariantBuilder_s1 without next usage to all your sys crates.

flukejones · 2017-07-06T10:17:53Z

guint julian : 1;
guint dmy : 1;

Hmm, I thought that was using the full guint, but declaring only 'n' bits of that integer were used... I'll have a look at the bitflags crate.

Also still not solved problem with including GVariantBuilder_u1 and GVariantBuilder_s1 without next usage to all your sys crates.

I don't understand what you mean... The result is;

#[repr(C)]
pub struct GVariantBuilder {
    pub u1: GVariantBuilder_u1,
}
 
#[repr(C)]
pub union GVariantBuilder_u1 {
    pub s1: GVariantBuilder_s1,
    pub x: [size_t; 16],
}
 
#[repr(C)]
pub struct GVariantBuilder_s1 {
    pub partial_magic: size_t,
    pub type_: *const GVariantType,
    pub y: [size_t; 14],
}

which to me seems fine. You would need to write the API on top of this to accommodate the nesting...
As far as I can tell, the end result is the same as the C definition? fields s1 and u1 are just addresses to the "anonymous" struct/union right?

I think the whole issue comes from in the gir;

   <record (trimmed info)>
      <union name="u" c:type="u">
        <record name="s" c:type="s">

I can't seem to get those names out, and if I did and the C_Type ended up being name 'u' or 's' then this would cause issues. I can make a small change to not add a number to those two fields, but then I'm not sure it that will cause issues later either.

Very sorry if I seem to be lacking an understanding on some things here.

EPashkin · 2017-07-06T10:29:30Z

Result is fine. Problem that it added to all sys crates not only on glib (7 unused copies for each these union and inner struct).

EPashkin · 2017-07-06T10:40:05Z

Same with GDoubleIEEE754_s1, GFloatIEEE754_s1.
Problem at seems that Library::record and Library::union add type to MAIN_NAMESPACE

flukejones · 2017-07-06T10:41:55Z

EDIT:

Is it that MAIN_NAMESPACE do you think? I couldn't get it going with INTERNAL_NAMESPACE, it wouldn't include the extra structs/unions.

I see we commented at almost the same time 😃

Regarding the bit field structs, I'll see if I can use bitflags crate.

EPashkin · 2017-07-06T10:46:55Z

Yes, it one of functions, that cause problem (I thought that it in Library). It need ns_id: u16 parameter.
Bitflags IMHO don't help with this and better just use "old" code with commenting.

flukejones · 2017-07-06T11:06:25Z

@EPashkin are you sure about the bitflags? Mozilla is using it with bindgen for their servo stuff, and the results seem to wok well.

EPashkin · 2017-07-06T11:25:14Z

You can try ;)

EPashkin · 2017-07-06T11:28:10Z

Strange, but duplicates still present :(

EPashkin · 2017-07-06T11:30:50Z

src/parser.rs

+
+    fn read_record(&mut self, parser: &mut Reader, ns_id: u16, attrs: &Attributes)
+        -> Result<Option<Type>> {
+        let name = try!(


let mut name

flukejones · 2017-07-06T11:40:46Z

I'm not seeing any duplicates here 😕

EPashkin · 2017-07-06T11:45:17Z

I meant that (for example) both atk-sys/src/lib.rs and glib-sys/src/lib.rs
have GVariantBuilder_u1, and it don't used in atk-sys at all.

EPashkin · 2017-07-06T11:53:30Z

Sorry, something bugged on my machine: duplicate actually removed.
Only last problem with bitsized structs

EPashkin · 2017-07-06T11:59:50Z

Minor bug in result formating:
https://github.com/gtk-rs/gir/pull/402/files#diff-0c37b0c87c3c91119e579feee853b5d0R368 only needed if feature disabled.

flukejones · 2017-07-06T12:11:56Z

Okay, that should be the final issue. Except the result formatting? I can't see what you are referring too.

EPashkin · 2017-07-06T12:15:07Z

Seems github generate broken links.
I meant last write in codegen/sys/lib_::generate_unions

    if !items.is_empty() {
        try!(writeln!(w, ""));
    }

EPashkin · 2017-07-06T12:23:15Z

Now produce strange results for GIOChannel, GScannerConfig, gobject-sys::GClosure. gtk-sys::GtkTextAttributes.
Before all after bit-sized fields was commented too.

#[repr(C)]
pub struct GIOChannel {
    pub ref_count: c_int,
    pub funcs: *mut GIOFuncs,
    pub encoding: *mut c_char,
    pub read_cd: GIConv,
    pub write_cd: GIConv,
    pub line_term: *mut c_char,
    pub line_term_len: c_uint,
    pub buf_size: size_t,
    pub read_buf: *mut GString,
    pub encoded_read_buf: *mut GString,
    pub write_buf: *mut GString,
    pub partial_write_buf: [c_char; 6],
    _truncated_record_marker: c_void,
    //use_buffer: guint: 1,
    //do_encode: guint: 1,
    //close_on_unref: guint: 1,
    //is_readable: guint: 1,
    //is_writeable: guint: 1,
    //is_seekable: guint: 1,
    pub reserved1: gpointer,
    pub reserved2: gpointer,
}

EPashkin · 2017-07-06T14:59:33Z

Hm, Seems rust-lang/rust#42068 already stabilized part of unions and it in already beta.
Glib has only problem with bit-sized GDoubleIEEE754_s1 and GFloatIEEE754_s1 as it non-Copy.

EPashkin · 2017-07-06T15:01:55Z

Good thing that I don't see usage GFloatIEEE754 and GDoubleIEEE754 in any functions and we can try ignore its after next release.

flukejones · 2017-07-06T20:58:13Z

We're almost there 😄

I see why the proper truncation is required for fields after bitflags. Just needed to allow either is_bits or !truncated.

EPashkin · 2017-07-07T04:57:49Z

Thanks, @Luke-Nukem, Now I don't see any potential problems in generated code.
Can you remove empty line after generate_unions declaration?
And also please squash this PR to one commit as there too many merges in it.

flukejones · 2017-07-07T05:48:31Z

I think I did the squash correctly? It's commit #f56da73.

EPashkin · 2017-07-07T05:54:31Z

Yes, #f56da73 is good, but next is not (it merge again).
Please, reset your master branch to this commit, and force push it to your repo.

flukejones · 2017-07-07T05:56:54Z

Got it. Figured if there was an issue it would be an easy solve. Never done a squash before, very handy.

flukejones · 2017-07-07T05:59:46Z

Hold fire a few minutes, I'll clean up the commit log

EPashkin · 2017-07-07T06:03:43Z

Actually you added to squashed commit some changes from current master (like adding external_libraries.rs and using it) but git is smart and will ignore it on merge.
Or you can do git rebase 9e3f4ccff85d1e to be sure that it contains only your changes.

flukejones · 2017-07-07T06:11:56Z

Okay, I think we're good now?

EPashkin · 2017-07-07T06:12:52Z

Previous was good (based on 9e3f4cc - current upstream master)

EPashkin · 2017-07-07T06:14:22Z

Just to git rebase 9e3f4ccff85d1e again

This commit enables full genertion of unions if using nightly Rust with feature "use-unions" enabled. It also enables successful generation of nested (anonymous) struct->union->struct.

EPashkin · 2017-07-07T06:17:19Z

Yes, now it good.
Thanks again for unions and readme clarification.
I wait for CI just it case and merge

EPashkin · 2017-07-07T06:18:02Z

And excuse me for a lot of nitpicking 😉

flukejones · 2017-07-07T06:20:27Z

Nitpicking is good. You caught a few things I just didn't think about (or know), or had missed. And it's good practice for me 😉

I'm looking forward to union stabilization in Rust. And when I get more time (or need it) I'll work on the bit-fields in multitype-structs.

EPashkin reviewed Jul 6, 2017

View reviewed changes

src/parser.rs Outdated

@@ -21,6 +21,7 @@ pub fn is_empty_c_type(c_type: &str) -> bool {

c_type == EMPTY_CTYPE

}

Copy link

Member

EPashkin Jul 6, 2017

Choose a reason for hiding this comment

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

Unneeded extra line

EPashkin reviewed Jul 6, 2017

View reviewed changes

flukejones force-pushed the master branch from 4fe5c08 to fa266f9 Compare July 6, 2017 20:57

flukejones force-pushed the master branch from fa266f9 to 0165ee3 Compare July 6, 2017 21:02

flukejones force-pushed the master branch from d043769 to f56da73 Compare July 7, 2017 05:55

flukejones force-pushed the master branch 3 times, most recently from 59a3d75 to 52be615 Compare July 7, 2017 06:10

Adds ability to generate unions and nested (anonymous) structs

78d6f9b

This commit enables full genertion of unions if using nightly Rust with feature "use-unions" enabled. It also enables successful generation of nested (anonymous) struct->union->struct.

flukejones force-pushed the master branch from 52be615 to 78d6f9b Compare July 7, 2017 06:15

EPashkin merged commit 2192ae7 into gtk-rs:master Jul 7, 2017

		@@ -21,6 +21,7 @@ pub fn is_empty_c_type(c_type: &str) -> bool {
		c_type == EMPTY_CTYPE
		}

Full enablement of Unions using nightly Rust #402

Full enablement of Unions using nightly Rust #402

Conversation

flukejones commented Jul 6, 2017

EPashkin commented Jul 6, 2017

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

flukejones commented Jul 6, 2017

EPashkin commented Jul 6, 2017

flukejones commented Jul 6, 2017

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 Jul 6, 2017

flukejones commented Jul 6, 2017 • edited Loading

EPashkin commented Jul 6, 2017

EPashkin commented Jul 6, 2017

flukejones commented Jul 6, 2017 • edited Loading

EPashkin commented Jul 6, 2017

flukejones commented Jul 6, 2017

EPashkin commented Jul 6, 2017

EPashkin commented Jul 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flukejones commented Jul 6, 2017

EPashkin commented Jul 6, 2017

EPashkin commented Jul 6, 2017

EPashkin commented Jul 6, 2017

flukejones commented Jul 6, 2017

EPashkin commented Jul 6, 2017

EPashkin commented Jul 6, 2017 • edited Loading

EPashkin commented Jul 6, 2017

EPashkin commented Jul 6, 2017

flukejones commented Jul 6, 2017

EPashkin commented Jul 7, 2017

flukejones commented Jul 7, 2017 • edited Loading

EPashkin commented Jul 7, 2017

flukejones commented Jul 7, 2017

flukejones commented Jul 7, 2017

EPashkin commented Jul 7, 2017

flukejones commented Jul 7, 2017

EPashkin commented Jul 7, 2017

EPashkin commented Jul 7, 2017

EPashkin commented Jul 7, 2017

EPashkin commented Jul 7, 2017

flukejones commented Jul 7, 2017

flukejones commented Jul 6, 2017 •

edited

Loading

flukejones commented Jul 6, 2017 •

edited

Loading

EPashkin commented Jul 6, 2017 •

edited

Loading

flukejones commented Jul 7, 2017 •

edited

Loading