Skip to content

Commit

Permalink
a few subtle logical and style improvements based on @kjvalencik's re…
Browse files Browse the repository at this point in the history
…view:

- 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
  • Loading branch information
dherman committed Nov 25, 2024
1 parent e41e317 commit 3031a97
Show file tree
Hide file tree
Showing 5 changed files with 27 additions and 19 deletions.
4 changes: 2 additions & 2 deletions crates/neon/src/sys/async_work.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ pub unsafe fn schedule<I, O, D>(
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()
}
}
Expand Down Expand Up @@ -151,7 +151,7 @@ unsafe extern "C" fn call_complete<I, O, D>(env: Env, status: napi::Status, data
} = *Box::<Data<I, O, D>>::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| {
Expand Down
36 changes: 22 additions & 14 deletions crates/neon/src/sys/object.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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")]
Expand All @@ -31,17 +31,17 @@ 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,
napi::KeyFilter::ALL_PROPERTIES | napi::KeyFilter::SKIP_SYMBOLS,
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();
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down
2 changes: 1 addition & 1 deletion crates/neon/src/sys/reference.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
}

Expand Down
2 changes: 1 addition & 1 deletion crates/neon/src/sys/tag.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down
2 changes: 1 addition & 1 deletion crates/neon/src/sys/tsfn.rs
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ impl<T> Drop for ThreadsafeFunction<T> {
}

unsafe {
assert_eq!(
debug_assert_eq!(
napi::release_threadsafe_function(
self.tsfn.0,
napi::ThreadsafeFunctionReleaseMode::Release,
Expand Down

0 comments on commit 3031a97

Please sign in to comment.