Skip to content
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

chore(neon): fix clippy lints #1085

Merged
merged 6 commits into from
Nov 26, 2024
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
a few subtle logical and style improvements based on @kjvalencik's re…
…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
dherman committed Nov 25, 2024
commit 3031a974c58fed8cd3483295d3d1cb657b6ed8c7
4 changes: 2 additions & 2 deletions crates/neon/src/sys/async_work.rs
Original file line number Diff line number Diff line change
@@ -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()
}
}
@@ -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| {
36 changes: 22 additions & 14 deletions crates/neon/src/sys/object.rs
Original file line number Diff line number Diff line change
@@ -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,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();
@@ -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
2 changes: 1 addition & 1 deletion crates/neon/src/sys/reference.rs
Original file line number Diff line number Diff line change
@@ -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();
}
}

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

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