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

Started to update to last rustc version (removing box closure) #177

Merged
merged 22 commits into from
Jan 24, 2015
Merged

Started to update to last rustc version (removing box closure) #177

merged 22 commits into from
Jan 24, 2015

Conversation

GuillaumeGomez
Copy link
Contributor

No description provided.

@GuillaumeGomez
Copy link
Contributor Author

Can you take a look at it @jeremyletang ?

@gsingh93
Copy link
Contributor

Bumping this. The alpha is out, so now is a good time to fix these build errors.

@GuillaumeGomez
Copy link
Contributor Author

It's kinda difficult to update a code which I didn't write. That's why it's taking so much time.

@gsingh93
Copy link
Contributor

No problem. I'll try to help if I get time and can figure anything out.

@GuillaumeGomez
Copy link
Contributor Author

It would be very appreciated ! Thanks !

@gsingh93
Copy link
Contributor

I commented out that macro error because I couldn't figure it out, and I continued to fix the other errors (we can come back to it later). My progress is tracked here: https://github.com/gsingh93/rgtk/tree/fix-build

One significant change was that ToCStr was removed, so we have to use CString::from_slice() now (this migration isn't complete yet). Another change is that CVec is gone. I'm not familiar with doing FFI, so I can't really figure out what that needs to be changed to.

@GuillaumeGomez
Copy link
Contributor Author

For the ToCStr and CVec stuff, I made bindings here and here. The functions are just the same. For an example, look my rust-fmod repo or my rust-GSL repo.

@jeremyletang
Copy link
Owner

@gsingh93, you should prefer the new way provide by the std to handle the c types, as they should be maintained forever in the standard distribution of Rust.

I will check the macro / closure stuff if you can't @gsingh93, as soon as I have time.

@gsingh93
Copy link
Contributor

My plan is to convert all the ToCStr to use CString, use @GuillaumeGomez's CVec bindings for now, fix all the errors I can, then figure out the CVec stuff later (I think I'll have to wrap those types in my own structs that have destructors).

After fixing a bunch more errors, I'm left with a lot of these errors:

src/gtk/macros.rs:37:18: 37:22 error: the trait `Copy` may not be implemented for this type; the type has a destructor [E0184]
src/gtk/macros.rs:37         #[derive(Copy)]

Not sure what the right fix is. Removing the Copy trait might break more things. I don't even know what breaking change this is related to.

@gsingh93
Copy link
Contributor

What is the best way to fix the Copy/Drop issues? Specifically what's going on is that we have this macro:

macro_rules! struct_Widget(
    ($gtk_struct:ident) => (
        #[derive(Copy)]
        pub struct $gtk_struct {
            pointer: *mut ffi::C_GtkWidget
        }
    );
);

Which creates a widget struct that derives copy, but some widgets also call the impl_drop!() macro. Copy and Drop can't be implemented on the same object for safety reasons.

@jeremyletang
Copy link
Owner

hi @gsingh93, we can change the macro and remove the #[derive(Copy)] then implement manually impl Copy for YourWidget {} or a Clone impl for the struct which implements Drop too.

@gsingh93
Copy link
Contributor

I think removing the Copy impl is probably the best thing to do, because we shouldn't be copying the pointer without adding to the reference count like we do in Clone. I've removed it for now.

My current progress is in this pull request: #180. I think I'm done working on this for a little while, so if either of you want to continue, you can consider starting from there.

@GuillaumeGomez
Copy link
Contributor Author

And it's now finished ! @jeremyletang, you can check !

@gsingh93
Copy link
Contributor

Nice! There was a macro in signals.rs that I had commented out. Don't forget to comment it back in and fix it before merging.

@GuillaumeGomez
Copy link
Contributor Author

For the macro, it'll wait. I'm way too tired for that. Just waiting to get travis answer.

jeremyletang added a commit that referenced this pull request Jan 24, 2015
Started to update to last rustc version (removing box closure)
@jeremyletang jeremyletang merged commit cf3c43a into jeremyletang:master Jan 24, 2015
@GuillaumeGomez GuillaumeGomez deleted the build-fix branch January 29, 2015 21:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants