neon-bindings / neon

Rust bindings for writing safe and fast native Node.js modules.
https://www.neon-bindings.com/
Apache License 2.0
7.98k stars 282 forks source link

Clean up transmutes in scope.rs #295

Open nox opened 6 years ago

nox commented 6 years ago

There is no need for transmutes in:

extern "C" fn chained_callback<'a, T, P, F>(out: &mut Box<Option<T>>,
                                            parent: &P,
                                            v8: *mut raw::EscapableHandleScope,
                                            f: Box<F>)
    where P: Scope<'a>,
          F: for<'inner> FnOnce(&mut ChainedScope<'inner, 'a>) -> T
{
    let mut chained = ChainedScope {
        isolate: parent.isolate(),
        active: RefCell::new(true),
        v8: v8,
        parent: PhantomData,
        phantom: PhantomData
    };
    let result = f(&mut chained);
    **out = Some(result);
}

fn chain<'a, T, S, F>(outer: &S, f: F) -> T
    where S: Scope<'a>,
          F: for<'inner> FnOnce(&mut ChainedScope<'inner, 'a>) -> T
{
    ensure_active(outer);
    let closure: Box<F> = Box::new(f);
    let callback: extern "C" fn(&mut Box<Option<T>>, &S, *mut raw::EscapableHandleScope, Box<F>) = chained_callback::<T, S, F>;
    let mut result: Box<Option<T>> = Box::new(None);
    {
        let out: &mut Box<Option<T>> = &mut result;
        { *outer.active_cell().borrow_mut() = false; }
        unsafe {
            let out: *mut c_void = mem::transmute(out);
            let closure: *mut c_void = mem::transmute(closure);
            let callback: extern "C" fn(&mut c_void, *mut c_void, *mut c_void, *mut c_void) = mem::transmute(callback);
            let this: *mut c_void = mem::transmute(outer);
            neon_runtime::scope::chained(out, closure, callback, this);
        }
        { *outer.active_cell().borrow_mut() = true; }
    }
    result.unwrap()
}

I also believe that the double indirection for the result is unnecessary, and that panics should be caught inside the callback.

This can be rewritten as:

fn chain<'a, T, S, F>(outer: &S, f: F) -> T
where
    S: Scope<'a>,
    F: for<'inner> FnOnce(&mut ChainedScope<'inner, 'a>) -> T,
{
   extern "C" fn callback<'a, T, P, F>(
        out: *mut c_void,
        parent: *mut c_void,
        v8: *mut c_void,
        f: *mut c_void,
    )
    where
        P: Scope<'a>,
        F: for<'inner> FnOnce(&mut ChainedScope<'inner, 'a>) -> T,
    {
        let out = unsafe { &mut *(out as *mut Option<T>) };
        let parent = unsafe { &*(parent as *const P) };
        let v8 = unsafe { &mut *(v8 as *mut raw::EscapableHandleScope) };
        let f = unsafe { Box::from_raw(f as *mut F) };
        let mut chained = ChainedScope {
            isolate: parent.isolate(),
            active: RefCell::new(true),
            v8: v8,
            parent: PhantomData,
            phantom: PhantomData
        };
        let result = f(&mut chained);
        *out = Some(result);
    }

    ensure_active(outer);
    let closure: Box<F> = Box::new(f);
    let mut result: Option<T> = None;
    {
        *outer.active_cell().borrow_mut() = false;
        unsafe {
            let out = &mut result as *mut Option<T> as *mut c_void;
            let closure = Box::into_raw(f) as *mut c_void;
            let this = outer as *const S as *mut c_void;
            neon_runtime::scope::chained(out, closure, callback::<T, S, F>, this);
        }
        *outer.active_cell().borrow_mut() = true;
    }
    result.unwrap()
}
nox commented 6 years ago

I think many many others transmute uses could be cleaned up too.