Use unboxed closures throghout the code.

[bitonic]

The tricky bit in in Timer, which now works a bit differently.
Specifically, we always stop the timer when the Timer object is
dropped, because otherwise the closure might run past its lifetime 'a,
rendering the code unsafe. I'm pretty sure the previous code is buggy.

Note that this pull request is towards the same goal as #263, but does things differently. I don't use TraitObjects -- I don't think it's necessary -- and I already remove the remove_on_drop option. I've been using rust for a week or so so review with care :).

---

Edit: Following the discussion below, I've decided to drop support for Rust closures with SDL timers, and use extern "C" functions instead.

[bitonic]

Oh, I also added some tests for the Timer.

[drbawb]

This segfaults on my Mac w/ the following example: https://gist.github.com/drbawb/3488d741c436e6be9d39

My PR also fails this test, though for different reasons. (The closure doesn't live long enough; but that shouldn't be an issue as your boxed closure should live as long as T.)

---

I believe to get the semantics you desire you probably need to use an FnOnce stored as a Thunk (well... a raw::TraitObject which points to the Thunk) and then call Thunk#invoke() in the C callback.

[drbawb]

When using &Fn() -> uint, the raw::TraitObject essentially lets me bypass a little borrow-check nit.
Without it you'd have to call Timer::new() as follows: Timer::new(10, &|&:| { /* closure content */ });
Apart from being kind of ugly: it fails the borrow-check even though the closure empirically lives long enough.

Taking the closure by value and doing the cast to a raw::TraitObject lets me side-step that and still allow timers to be defined in one line.

---

Though after further testing you are correct that it's possible for me to define a Timer which outlives the closure itself. -- For some reason your code with Box<Fn() -> uint> also segfaults w/ the same test. (I've posted the example to your PR.)

[bitonic]

Yeah, that gist segfaults here too. Bizarrely, if you insert a sdl2::timer::delay(4000) after the timer.start(), things work smoothly. So it seems like the closure is collected after the tick function exits, but it shouldn't be.

Regarding TraitObject, I'd have to try and play with the code to fully understand -- I'm still getting used to Rust's rules regarding this stuff...

Another thing: I'm using FnOnce where possible and FnMut in this timer stuff, since I'd expect it to be a common use case to modify some shared state when a timer expires.

[bitonic]

Oh, I know what the problem is -- I'm casting back a reference to Box which is on the stack local to start and is invalid afterwards. I'll think about the right way of doing this.

[bitonic]

Changing the closure field to closure: &'a Box<FnMut() -> uint + 'a> does the trick but is really quite unpleasant. I don't know if there is another way to get a pointer to an unboxed closure as an argument. Maybe using TraitObject, but I don't quite understand its subtleties.

[drbawb]

I've been playing with this; I think is about as close as we can get to soundness.
I was able to use raw::TraitObject to improve the ergonomics slightly -- the caller no longer needs to annotate the closure type or pass it by reference.

It passes both your test cases as well (after the mentioned improvements of course.)

See: drbawb/rust-sdl2@679ba12

---

There's still one thing that worries me, per the SDL2 documentation: SDL_AddTimer: Use this function to set up a callback function to be run on a separate thread after the specified number of milliseconds has elapsed.

The timers are apparently run on a separate thread; I think we should be using closures which own their environment here (move|| {} aka FnOnce). The closure would still have a mutable environment, but per the namesake it could only be called once. -- In order to have periodic timers you would need to have closures returning closures. I'm playing with it, but it's not pretty.

[bitonic]

Ah, you are right about moving the closure. That's nasty.

Maybe the best option is to stay close to the SDL interface and let the user define his own extern if he really needs to use SDL timers. To be fair, given the capabilities of the Rust runtime, you can easily define timers yourself in a portable way, so I'm not sure that spending a lot of effort trying to make this work is worth it. After all, one of the reason we don't want to use C is so that we can do write this sort of programs easily :).

[bitonic]

This is what the interface would look like: bitonic@aa89bc0 . I think it's the way to go, instead of bending over backwards to try to mix C functions and rust closures.

[drbawb]

Looks good to me. -- By exposing callback and param I think it's even possible for people to implement a wrapper type which mimics the old behavior if they need it.

[drbawb]

I still feel free-standing callbacks are the way to go; but I've sort of figured something out in the course of playing with timer callbacks that capture-by-move.

Even after implementing drbawb/rust-sdl2@7bb2e528 -- I was still getting errors under the following scenario:

1. Create timer in free standing function
2. Start timer
3. Return timer (by value)
4. Program crashes when the timer fires the callback

(On the bright-side: my capture-by-move did make this a runtime error instead of a segfault. So at least I got memory safety back!)

---

Hindsight being 20/20 the problem is a little clearer now: the Timer cannot move once #start() has been called.

What is happening is that the Timer (and thus the location of our Box<Fn>) is moving when Timer is returned from a function. -- If Timer#start() is called then the SDL library now has a stale pointer to a closure which has been moved elsewhere in memory; ergo segmentation faults.

Thus a truly safe API for this would simply need to require that once Timer#start() is called the structure containing the closure cannot be moved by value. Not sure how to enforce that statically; but it at least seems doable.

---

Food for thought in-case we want to iterate on a safe API for SDL timers.

[AngryLawyer]

I'm pretty happy that people much clever than I am are sorting this out - are you happy for this to be merged?

[bitonic]

I didn't follow @drbawb last investigations, but I'm happy to merge as-is.

After all, the timer functionality can easily and more idiomatically replicated with a

Thread::spawn(move || {
    loop {
        ...
        sdl2::delay(...);
    }
});

so as said previously I don't think this is an issue spending time on.

[AngryLawyer]

Wonderful. In we go