Skip to content

Use more lifetimes #41

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

Closed
wants to merge 1 commit into from

Conversation

Manishearth
Copy link

Currently this crate has FFI functions which have no lifetimes in the arguments but a lifetime in the return type.

This currently compiles, but is a bug -- you can't do the same for non-FFI functions, for example.

Nor does it make sense, such a function will return an unbounded lifetime.

On the latest nightly this causes ICEs, and one of the possible fixes is to turn this into a hard error as we do for non FFI functions.

I added some lifetimes so that it will elide correctly. It would be possible to implement Deref for StructPtr and Base, however currently they have public pointer fields so it is not possible to dereference these safely. Instead I added an unsafe borrow() method that cleanly gives you the contents as a pointer with the right lifetimes. I can clean this up further if you'd like.

@SimonSapin
Copy link

@Manishearth This PR only includes changes to src/base.rs. Did you also make changes to .gitignore’d files generated by a Python script?

@Manishearth
Copy link
Author

yeah, I did :|

@SimonSapin
Copy link

@Manishearth
Copy link
Author

Updated; works now.

It uses lifetimes in a few more situations than necessary but that's just easier for autogenerated code. Still safe.

@Manishearth
Copy link
Author

I'm getting a lot of unrelated test failures locally

Here's a patch that fixes one of the examples:

diff --git a/examples/must_fail_borrow_check__setup.rs b/examples/must_fail_borrow_check__setup.rs
index 719b2ad..9f2266a 100644
--- a/examples/must_fail_borrow_check__setup.rs
+++ b/examples/must_fail_borrow_check__setup.rs
@@ -4,10 +4,12 @@ use std::iter::{Iterator};
 
 fn main() {
 
+    let conn;
     let setup;
     let screen_num;
     {
-        let (conn, sn) = xcb::Connection::connect(None).unwrap();
+        let (conn_, sn) = xcb::Connection::connect(None).unwrap();
+        conn = conn_;
         setup = conn.get_setup();
         screen_num = sn;
     }

@Manishearth
Copy link
Author

Oh, wait, no, that example is supposed to fail.

@Manishearth
Copy link
Author

So there's this other xkb crate with similar issues.

I'm not really familiar with this bindings generation setup and I'm unsure why my changes didn't work for the xkb stuff so I'm going to leave it as is.

@SimonSapin
Copy link

@rtbo, would you agree to backport this change to a 0.7.x version? Otherwise to get this fix in Servo we’d need to make changes through the dependency chain of clipboard and x11-clipboard.

@SimonSapin
Copy link

Ah, it turns out this PR needs some more changes. As it is, compiling on Nightly still fails like in #40. Travis is green because the build with Rust Nightly is an "allowed failure".

@rtbo
Copy link
Collaborator

rtbo commented Aug 13, 2017

@SimonSapin Yes, fixing both 0.7.x and 0.8.x is no problem.

@SimonSapin
Copy link

@rtbo Manish and I didn’t manage to make the code generation script generate appropriate code to make this build on Nightly. Can you have a look?

@rtbo
Copy link
Collaborator

rtbo commented Aug 13, 2017

@Manishearth @SimonSapin I pushed a37d1fa that does the necessary lifetime assignments. (still need confirmation by you servo guys).
Also it's branched from 0.7.x.

@rtbo
Copy link
Collaborator

rtbo commented Dec 2, 2017

Addressed by rtbo@a37d1fa.
Closing

@rtbo rtbo closed this Dec 2, 2017
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