From 3031a974c58fed8cd3483295d3d1cb657b6ed8c7 Mon Sep 17 00:00:00 2001 From: David Herman Date: Mon, 25 Nov 2024 10:37:54 -0800 Subject: [PATCH] a few subtle logical and style improvements based on @kjvalencik's review: - style: don't `assert_eq!` of `Ok(())`, just `.unwrap()` - style: capitalize no_panic error message strings - logic: don't unwrap deleting async work if we're about to fail with a more clear error anyway - logic: when returning false to propagate exceptions, only do so on PendingException; panic on other errors --- crates/neon/src/sys/async_work.rs | 4 ++-- crates/neon/src/sys/object.rs | 36 +++++++++++++++++++------------ crates/neon/src/sys/reference.rs | 2 +- crates/neon/src/sys/tag.rs | 2 +- crates/neon/src/sys/tsfn.rs | 2 +- 5 files changed, 27 insertions(+), 19 deletions(-) diff --git a/crates/neon/src/sys/async_work.rs b/crates/neon/src/sys/async_work.rs index db668d507..338319f37 100644 --- a/crates/neon/src/sys/async_work.rs +++ b/crates/neon/src/sys/async_work.rs @@ -74,7 +74,7 @@ pub unsafe fn schedule( Ok(()) => {} status => { // If queueing failed, delete the work to prevent a leak - napi::delete_async_work(env, *work).unwrap(); + let _ = napi::delete_async_work(env, *work); status.unwrap() } } @@ -151,7 +151,7 @@ unsafe extern "C" fn call_complete(env: Env, status: napi::Status, data } = *Box::>::from_raw(data.cast()); if napi::delete_async_work(env, work).is_err() { - panic!("failed to delete async work"); + panic!("Failed to delete async work"); } BOUNDARY.catch_failure(env, None, move |env| { diff --git a/crates/neon/src/sys/object.rs b/crates/neon/src/sys/object.rs index fe049c98f..fe6ba1526 100644 --- a/crates/neon/src/sys/object.rs +++ b/crates/neon/src/sys/object.rs @@ -7,7 +7,7 @@ use super::{ /// Mutates the `out` argument to refer to a `napi_value` containing a newly created JavaScript Object. pub unsafe fn new(out: &mut Local, env: Env) { - assert_eq!(napi::create_object(env, out as *mut _), Ok(())); + napi::create_object(env, out as *mut _).unwrap(); } #[cfg(feature = "napi-8")] @@ -31,7 +31,7 @@ pub unsafe fn seal(env: Env, obj: Local) -> Result<(), napi::Status> { pub unsafe fn get_own_property_names(out: &mut Local, env: Env, object: Local) -> bool { let mut property_names = MaybeUninit::uninit(); - if napi::get_all_property_names( + match napi::get_all_property_names( env, object, napi::KeyCollectionMode::OwnOnly, @@ -39,9 +39,9 @@ pub unsafe fn get_own_property_names(out: &mut Local, env: Env, object: Local) - napi::KeyConversion::NumbersToStrings, property_names.as_mut_ptr(), ) - .is_err() { - return false; + Err(napi::Status::PendingException) => return false, + status => status.unwrap(), } *out = property_names.assume_init(); @@ -82,13 +82,15 @@ pub unsafe fn get_string( // Not using `crate::string::new()` because it requires a _reference_ to a Local, // while we only have uninitialized memory. - if napi::create_string_utf8(env, key as *const _, len as usize, key_val.as_mut_ptr()).is_err() { - return false; + match napi::create_string_utf8(env, key as *const _, len as usize, key_val.as_mut_ptr()) { + Err(napi::Status::PendingException) => return false, + status => status.unwrap(), } // Not using napi_get_named_property() because the `key` may not be null terminated. - if napi::get_property(env, object, key_val.assume_init(), out as *mut _).is_err() { - return false; + match napi::get_property(env, object, key_val.assume_init(), out as *mut _) { + Err(napi::Status::PendingException) => return false, + status => status.unwrap(), } true @@ -112,14 +114,20 @@ pub unsafe fn set_string( *out = true; - if napi::create_string_utf8(env, key as *const _, len as usize, key_val.as_mut_ptr()).is_err() { - *out = false; - return false; + match napi::create_string_utf8(env, key as *const _, len as usize, key_val.as_mut_ptr()) { + Err(napi::Status::PendingException) => { + *out = false; + return false; + } + status => status.unwrap(), } - if napi::set_property(env, object, key_val.assume_init(), val).is_err() { - *out = false; - return false; + match napi::set_property(env, object, key_val.assume_init(), val) { + Err(napi::Status::PendingException) => { + *out = false; + return false; + } + status => status.unwrap(), } true diff --git a/crates/neon/src/sys/reference.rs b/crates/neon/src/sys/reference.rs index a290cdd8a..b3d744153 100644 --- a/crates/neon/src/sys/reference.rs +++ b/crates/neon/src/sys/reference.rs @@ -31,7 +31,7 @@ pub unsafe fn unreference(env: Env, value: napi::Ref) { napi::reference_unref(env, value, result.as_mut_ptr()).unwrap(); if result.assume_init() == 0 { - assert_eq!(napi::delete_reference(env, value), Ok(())); + napi::delete_reference(env, value).unwrap(); } } diff --git a/crates/neon/src/sys/tag.rs b/crates/neon/src/sys/tag.rs index 3dcc888c0..588d7c71d 100644 --- a/crates/neon/src/sys/tag.rs +++ b/crates/neon/src/sys/tag.rs @@ -39,7 +39,7 @@ pub unsafe fn is_object(env: Env, val: Local) -> bool { pub unsafe fn is_array(env: Env, val: Local) -> bool { let mut result = false; - assert_eq!(napi::is_array(env, val, &mut result as *mut _), Ok(())); + napi::is_array(env, val, &mut result as *mut _).unwrap(); result } diff --git a/crates/neon/src/sys/tsfn.rs b/crates/neon/src/sys/tsfn.rs index ce793b7f5..7810244bd 100644 --- a/crates/neon/src/sys/tsfn.rs +++ b/crates/neon/src/sys/tsfn.rs @@ -182,7 +182,7 @@ impl Drop for ThreadsafeFunction { } unsafe { - assert_eq!( + debug_assert_eq!( napi::release_threadsafe_function( self.tsfn.0, napi::ThreadsafeFunctionReleaseMode::Release,