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

Unions: Remove feature-gate for stable rustc #415

Merged
merged 5 commits into from Feb 1, 2018
Merged

Conversation

@flukejones
Copy link
Contributor

@flukejones flukejones commented Jul 21, 2017

Removes all feature-gating of union, and now requires rust 1.19
Updates readme to reflect this.

@EPashkin
Copy link
Member

@EPashkin EPashkin commented Jul 22, 2017

Travis build failure is legit

let v = vec![
u,
"#![feature(untagged_unions)]",

This comment has been minimized.

@EPashkin

EPashkin Jul 22, 2017
Member

This line unneeded just "" instead

This comment has been minimized.

@flukejones

flukejones Jul 22, 2017
Author Contributor

Ah, yes. I was using nightly rust still.

@EPashkin
Copy link
Member

@EPashkin EPashkin commented Jul 22, 2017

Only last problem with error: unions with non-Copy fields are unstable (see issue #32836)
I thought that we can just add #[derive(Clone, Copy)] to child structs, but in case of GDoubleIEEE754_mpn it won't works because _truncated_record_marker: c_void.
Maybe we need change its type to u8.
Also from gdk's "GdkEvent" seems we need derive Copy for many child structs and it inners (GdkRectangle) etc.,
so IMHO simpler derive Copy for all structs and possibly for unions.
@GuillaumeGomez you agree with this?
@sdroege As I remember you was for derive Copy from start?

@flukejones
Copy link
Contributor Author

@flukejones flukejones commented Jul 22, 2017

Not sure how to go about #[derive(Clone, Copy)]... To be honest, I don't think we should be adding this manually as this implies that we are checking each case to be sure all fields can be Copy - which we are not doing unless it is at the parser/output level.

Changing _truncated_record_marker: c_void to u8 would be a good idea I think. I'll work on removing the #![feature(untagged_unions)] and implementing #[derive(Copy, Clone)] for those particular types. Do you have any suggestions for tracking which ones need it?

@EPashkin
Copy link
Member

@EPashkin EPashkin commented Jul 22, 2017

@Luke-Nukem Lets wait for other say their options. For GdkEvent need or implement recursive check and update for all children or just derive for all structs.

@flukejones
Copy link
Contributor Author

@flukejones flukejones commented Jul 22, 2017

@EPashkin no problem. I do know we absolutely shouldn't (and can't) implement #[derive(Copy, Clone)] for all structs, it would introduce inefficiencies such as copying large data structures automatically instead of using a move. Looking at the structure of what we use to output the bindings, there doesn't seem to be anything easily used for it - unless perhaps we insert some flag at the parse stage?

@EPashkin
Copy link
Member

@EPashkin EPashkin commented Jul 22, 2017

Hm, I don't though that some structures can be large and Copy for these unreasonable.
Maybe we can add some config for Copy and for expanding enum.
Or really check\add Copy flag all unions children and hope that its will not cause problem.

@flukejones flukejones force-pushed the flukejones:master branch 3 times, most recently from 83d94a7 to e9a7910 Jul 22, 2017
@EPashkin
Copy link
Member

@EPashkin EPashkin commented Jul 22, 2017

last commit produce strange diff:

// Records
 #[repr(C)]
-pub struct GArray {
+GArraypub struct GArray {
     pub data: *mut c_char,
     pub len: c_uint,
 }
@flukejones
Copy link
Contributor Author

@flukejones flukejones commented Jul 22, 2017

Oh dear, looks like I'm making a mess of git. Hang on, I'll try and fix this up.

@flukejones
Copy link
Contributor Author

@flukejones flukejones commented Jul 22, 2017

The sys crates I've compiled so far seem to be fine now.

@@ -434,7 +434,7 @@ fn generate_records(w: &mut Write, env: &Env, records: &[&Record]) -> Result<()>
if record.derive_copy {
try!(writeln!(w, "{}#[derive(Copy,Clone)]", comment));
}
try!(writeln!(w, "{0}pub struct {} {{", record.c_type));
try!(writeln!(w, "pub struct {} {{", record.c_type));

This comment has been minimized.

@EPashkin

EPashkin Jul 22, 2017
Member

This not correct too, you lost comment text

This comment has been minimized.

@flukejones

flukejones Jul 22, 2017
Author Contributor

🤦‍ 🤦‍ 🤦‍
Added back, squashed last series of commits. Should be good now.

@flukejones flukejones force-pushed the flukejones:master branch from 4b5b88e to a703885 Jul 22, 2017
@EPashkin
Copy link
Member

@EPashkin EPashkin commented Jul 22, 2017

Only last questionable problems with GdkEvent and GtkTextAppearance_u1, for this better wait opinions from others.

@EPashkin
Copy link
Member

@EPashkin EPashkin commented Jul 22, 2017

With GtkTextAppearance_u1 is problem that it used gdk::GdkRGBA so we can't add Copy for it automaticly.

@EPashkin
Copy link
Member

@EPashkin EPashkin commented Jul 22, 2017

Seems GtkTextAppearance_u1 actually unused as GtkTextAppearance is struct with bit-sized fields and truncated before union.

@flukejones
Copy link
Contributor Author

@flukejones flukejones commented Jul 22, 2017

Seems GtkTextAppearance_u1 actually unused as GtkTextAppearance is struct with bit-sized fields and truncated before union.

I guess that's just a side effect of how things are parsed then read out. Not sure what we can do about it other than just leave it. Or add in a special case for it? Do you think that is the only problem area?

@sdroege
Copy link
Member

@sdroege sdroege commented Jul 22, 2017

Just short comments

  • u8 or array thereof for bit fields makes sense. Just count the bits
  • I think deriving copy for all ffi structs is probably safe. As in, except for very few exceptions you only get them as pointers anyway. Using a configuration seems cleaner though, but OTOH we need to do that automatically for nested structs anyway. Copy for structs with pointers seems dangerous, but this is all unsafe code anyway so maybe OK. And in C, all structs are "Copy" too. Performance-wide I see no problem here in any case: if your code moved before, it will still do that. You might just copy without noticing, but that's only a problem if you have the whole struct and not just a pointer to it.

So in summary, I don't know. Making all Copy seems fine to me, a configuration also. Both are ugly in their own ways and in practice it probably makes no difference

@flukejones
Copy link
Contributor Author

@flukejones flukejones commented Jul 22, 2017

@sdroege the one problem with making all Copy though, is that there will be some things that shouldn't be or are inherently non-copy. I think c_void is one of those so if any structs have that as a field type then we'll have issues. Having said that I know c_void is u8 sized, and don't quite understand the purpose of c_void.

Copy on pointers... iirc raw pointers are fine and safe for copy, it's only when we deref them that it's unsafe. (?)

@sdroege
Copy link
Member

@sdroege sdroege commented Jul 22, 2017

For pointers, correct. You just need to be careful to free all copies later, I.e. do double-frees. But that's already with plain pointers

@EPashkin
Copy link
Member

@EPashkin EPashkin commented Jul 28, 2017

@GuillaumeGomez what you think about implementing Copy for almost all structs in sys crates to make unions work on stable?

@GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Jul 28, 2017

That's something I proposed not that long ago so I'm totally for it!

@EPashkin
Copy link
Member

@EPashkin EPashkin commented Jul 28, 2017

@Luke-Nukem sorry for delay, I thought that I already asked.

@flukejones
Copy link
Contributor Author

@flukejones flukejones commented Jul 29, 2017

Sorry about the delay here, I'll push through an update soon. Was a bit tied up with study this week.

@flukejones
Copy link
Contributor Author

@flukejones flukejones commented Jul 29, 2017

I'm not sure about pub struct Volatile<T>(T); being copy,clone... Thoughts?

Appveyor is pretty far behind in rust versions isn't it...

@flukejones
Copy link
Contributor Author

@flukejones flukejones commented Jul 29, 2017

error[E0277]: the trait bound `[f64; 128]: std::clone::Clone` is not satisfied
    --> /home/travis/build/gtk-rs/gir/tests/sys/gdk-sys/src/lib.rs:3743:5
     |
3743 |     pub axes: [c_double; 128],
     |     ^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `std::clone::Clone` is not implemented for `[f64; 128]`
     |
     = help: the following implementations were found:
               <[T; 28] as std::clone::Clone>
               <[T; 14] as std::clone::Clone>
               <[T; 7] as std::clone::Clone>
               <[T; 8] as std::clone::Clone>
             and 29 others
     = note: required by `std::clone::AssertParamIsClone`

Suggestions?

@EPashkin
Copy link
Member

@EPashkin EPashkin commented Jul 29, 2017

About old appveyour version: added fix #420

@EPashkin
Copy link
Member

@EPashkin EPashkin commented Jul 29, 2017

As gtk has single problem too, IMHO better add config parameter for object like "dont_copy=true"

error[E0277]: the trait bound `unsafe extern "C" fn(*mut GtkStyle, *mut cairo::cairo_t, GtkStateType, GtkShadowType, *mut GtkWidget, *const i8, i32, i32, i32, i32, GtkPositionType, i32, i32): std::clone::Clone` is not satisfied
    --> d:/eap/rust/0/sys/gtk-sys\src\lib.rs:5295:5
     |
5295 |     pub draw_shadow_gap: Option<unsafe extern "C" fn(*mut GtkStyle, *mut cairo::cairo_t, GtkStateType, GtkShadowType, *mut GtkWidget, *const c_char, c_int, c_int, c_int, c_int, GtkPositionType, c_int, c_int)>,
     |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `std::clone::Clone` is not implemented for `unsafe extern "C" fn(*mut GtkStyle, *mut cairo::cairo_t, GtkStateType, GtkShadowType, *mut GtkWidget, *const i8, i32, i32, i32, i32, GtkPositionType, i32, i32)`
     |
     = note: required because of the requirements on the impl of `std::clone::Clone` for `std::option::Option<unsafe extern "C" fn(*mut GtkStyle, *mut cairo::cairo_t, GtkStateType, GtkShadowType, *mut GtkWidget, *const i8, i32, i32, i32, i32, GtkPositionType, i32, i32)>`
     = note: required by `std::clone::AssertParamIsClone`
@@ -574,7 +516,7 @@ fn generate_fields(env: &Env, struct_name: &str, fields: &[Field]) -> (Vec<Strin
if c_type.is_err() {
commented = true;
}
if !cfg!(feature = "use_unions") && is_gvalue && field.name == "data" {
if is_gvalue && field.name == "data" {

This comment has been minimized.

@EPashkin

EPashkin Jul 29, 2017
Member

Why this don't removed?

@@ -565,7 +507,7 @@ fn generate_fields(env: &Env, struct_name: &str, fields: &[Field]) -> (Vec<Strin
continue 'fields;
}

if is_gweakref && !cfg!(feature = "use_unions") {
if is_gweakref {

This comment has been minimized.

@EPashkin

EPashkin Jul 29, 2017
Member

Why this don't removed?

@@ -444,7 +417,7 @@ fn generate_records(w: &mut Write, env: &Env, records: &[&Record]) -> Result<()>
if lines.is_empty() {
try!(writeln!(
w,
"{}#[repr(C)]\n{0}pub struct {}(c_void);\n",
"{0}#[repr(C)]\n{0}#[derive(Copy,Clone)]\n{0}pub struct {1}(u8);\n",

This comment has been minimized.

@EPashkin

EPashkin Jul 29, 2017
Member

This better changed back (it will produce only 1 extra "dont_copy")

This comment has been minimized.

@EPashkin

EPashkin Jul 29, 2017
Member

..because it just don't right, copy record with unknown size

This comment has been minimized.

@flukejones

flukejones Jul 29, 2017
Author Contributor

I'm not sure I understand, pub struct GIConv(u8); should not be copy?

This comment has been minimized.

@EPashkin

EPashkin Jul 29, 2017
Member

Yes, with GIOChannel

This comment has been minimized.

@sdroege

sdroege Jul 29, 2017
Member

Not sure I understand either. What is the broken code generated by this?

This comment has been minimized.

@EPashkin

EPashkin Jul 29, 2017
Member

this case produce warning: found non-foreign-function-safe member in struct marked #[repr(C)]: found Rust tuple type in foreign module; consider using a struct instead
just struct produce warning: found zero-size struct in foreign module, consider adding a member to this struct

This comment has been minimized.

@sdroege

sdroege Jul 29, 2017
Member

I see... a case for the new #[repr(transparent)] I guess

This comment has been minimized.

@EPashkin

EPashkin Jul 29, 2017
Member

Sorry, now I don't understand how it related to struct without fields.

This comment has been minimized.

@sdroege

sdroege Jul 29, 2017
Member

You would want to represent an opaque C type. It's not really a struct without fields, it's just that the struct definition is unknown from the headers. So you can only ever use it as a pointer.

But that's actually extern type: rust-lang/rfcs#1861

This comment has been minimized.

@EPashkin

EPashkin Jul 29, 2017
Member

extern type is that we need for this case but it not realized and may cause build error when inserted in other struct directly (as with GIConv)
#[repr(transparent)] can be used for minimize alignment errors for known fields, so also not for this case, so last repr(C).
I prefer c_void over u8 as it don't implement Copy while has same size and alignment.

@EPashkin
Copy link
Member

@EPashkin EPashkin commented Jul 29, 2017

About Volatile: as it applied only to simple types it can be copy.

if lines.is_empty() {
try!(writeln!(
w,
"{0}#[repr(C)]\n{0}#[derive(Copy,Clone)]\n{0}pub union {1}(c_void);\n",

This comment has been minimized.

@EPashkin

EPashkin Jul 29, 2017
Member

Copy better removed as for records
PS. this code actually unused.

@flukejones
Copy link
Contributor Author

@flukejones flukejones commented Jul 29, 2017

config parameter for object like "dont_copy=true"

@EPashkin can you advise where I would look for examples of how to do this?

@flukejones flukejones force-pushed the flukejones:master branch from b86b995 to c199ba4 Jul 29, 2017
@EPashkin
Copy link
Member

@EPashkin EPashkin commented Jul 29, 2017

Add field to GObject same as https://github.com/gtk-rs/gir/blob/master/src/config/gobjects.rs#L151
then get it in generate_records

let full_name = format!("{}.{}", env.namespaces.main().name, record.name);
let config = env.config.objects.get(&full_name);
@EPashkin
Copy link
Member

@EPashkin EPashkin commented Jul 29, 2017

.. As config can be None for ffi, use if-let or config.map(|o| o.dont_copy).unwrap_or(false) to get value.

@flukejones
Copy link
Contributor Author

@flukejones flukejones commented Jul 30, 2017

I need to put this on the backlog for a week or so while I do a few other things of importance. I may work on it off/on through the week however, we'll see.

@sdroege
Copy link
Member

@sdroege sdroege commented Sep 18, 2017

What's the status here?

@flukejones
Copy link
Contributor Author

@flukejones flukejones commented Sep 18, 2017

@flukejones flukejones force-pushed the flukejones:master branch from c199ba4 to ac06389 Oct 25, 2017
@flukejones
Copy link
Contributor Author

@flukejones flukejones commented Oct 25, 2017

@sdroege @EPashkin I've done a rebase of this on to master and run a couple of small gir tests - seems okay.

Looking back at previous comments it looks like I still have a few things to do such as "dont_copy=true". Is this still a blocker?

@sdroege
Copy link
Member

@sdroege sdroege commented Oct 25, 2017

IMHO just having copy on all FFI things is fine, configuration can be added later once needed :)

@flukejones
Copy link
Contributor Author

@flukejones flukejones commented Oct 25, 2017

@flukejones flukejones force-pushed the flukejones:master branch from ac06389 to 612089f Nov 13, 2017
@flukejones flukejones force-pushed the flukejones:master branch from 612089f to 8560db7 Nov 13, 2017
@flukejones
Copy link
Contributor Author

@flukejones flukejones commented Nov 13, 2017

Tests will pass with this commit to cairo.

I get the impression that this is going to require a more in-depth solution rather than just implement Copy,Clone for all. Perhaps by marking unions in codegen and everything branching from it, and only derive/implement Copy,Clone for those, along with setting up the correct sized type to replace c_void for each architecture (c_void does not allow Copy).

@ghost ghost mentioned this pull request Jan 29, 2018
@EPashkin EPashkin merged commit 0e6665f into gtk-rs:master Feb 1, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ghost ghost mentioned this pull request Mar 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.