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

internal: add now ffi #553

Merged
merged 1 commit into from
Jan 7, 2025
Merged

internal: add now ffi #553

merged 1 commit into from
Jan 7, 2025

Conversation

Young-Flash
Copy link
Collaborator

No description provided.

Copy link

‼️ This code review is generated by a bot. Please verify the content before trusting it.

Here are three observations and potential issues from the provided git diff output:

  1. Inconsistent Time Unit Handling:

    • In the now function, the time is being converted to milliseconds (duration.as_millis()), while in the instant_now function (not shown in the diff but likely exists in the codebase), the time might be handled in a different unit (e.g., nanoseconds or seconds). This inconsistency could lead to confusion or bugs when comparing or using timestamps across different parts of the codebase. Ensure that all time-related functions use the same unit for consistency.
  2. Error Handling in result.set_index:

    • The result.set_index(scope, 0, secs).unwrap(); line in the now function uses unwrap(), which will panic if the operation fails. This could crash the program if the set_index call fails for any reason. Consider using proper error handling, such as logging the error or propagating it, instead of panicking.
  3. Redundant into() Calls:

    • The line let secs = v8::Number::new(scope, duration.as_millis() as f64).into(); includes an unnecessary .into() call. The v8::Number::new function already returns a v8::Local<v8::Number>, which is the correct type for set_index. The .into() call is redundant and can be removed for cleaner code.

These are the key issues I noticed. Let me know if you need further clarification or assistance!

@Young-Flash Young-Flash enabled auto-merge January 7, 2025 08:58
@Young-Flash Young-Flash merged commit 099c4c7 into main Jan 7, 2025
14 checks passed
@Young-Flash Young-Flash deleted the now_ffi branch January 7, 2025 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant