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

[refactor] Update the APIs related to sync host function creation #26

Merged
merged 2 commits into from
Jul 21, 2023

Conversation

apepkuss
Copy link
Collaborator

In this PR, the following APIs are refactored:

  • wasmedge-sdk
    • Func::new, Func::wrap_func
    • ImportObjectBuilder::with_func, ImportObjectBuilder::with_func_by_type
    • PluginModuleBuilder::with_func
  • wasmedge-sys
    • Function::create_sync_func

Copy link
Member

juntao commented Jul 21, 2023

Hello, I am a code review bot on flows.network. Here are my reviews of code commits in this PR.


Most important findings:

  • The change from Box<T> to &mut T for the data parameter in the create_sync_func method seems to be a significant improvement in the API for creating synchronous host functions. It allows for more flexible and efficient usage of mutable data.
  • The sys_host_function attribute added to the real_add function in the context_data_to_host_func.rs file is a notable addition. However, it lacks necessary documentation comments explaining its purpose and usage.
  • There are minor issues with formatting and code cleanliness in the context_data_to_host_func.rs file, such as missing currency symbols in println statements and unnecessary imports. These should be addressed for better code readability and maintainability.
  • The method signature for the create_sync_func method in the function.rs file should be updated to use the Rust naming convention of camelCase instead of snake_case.

Overall, the pull request brings improvements to the API for creating synchronous host functions while introducing some minor issues with formatting and code cleanliness. It would be beneficial to address these issues and provide more documentation or comments to clarify the purpose and impact of the changes.

Details

Commit 3b15b6d975ecbf4d31cb9527bc0ae8edb67dc176

Key changes made in the pull request are:

  1. Updated the Function::create_sync_func method in the function.rs file to accept a mutable reference (&mut) instead of a boxed type (Box<T>) for the data parameter.
  2. Added the sys_host_function attribute to the real_add function in the context_data_to_host_func.rs file.

Potential problems:

  1. In the context_data_to_host_func.rs file, the println!("data: {data:?}") statement is missing the $ symbol before the data variable. It should be println!("data: {data:?}").
  2. The println!("data: {data:?}") statement in the real_add function in the context_data_to_host_func.rs file is using the wrong braces format. It should be println!("data: {:?}", data).
  3. The new sys_host_function attribute added to the real_add function in the context_data_to_host_func.rs file is missing a documentation comment explaining its purpose and usage.
  4. In the function.rs file, the create_sync_func method's signature should be updated to use the Rust naming convention of camelCase instead of snake_case. It should be createSyncFunc.
  5. The code in the context_data_to_host_func.rs file does not conform to Rust's formatting conventions and contains unnecessary imports. These should be cleaned up.

Overall, the changes seem to improve the API for creating synchronous host functions by accepting mutable references instead of boxed types for the data parameter. However, there are some minor issues with formatting and code cleanliness that should be addressed.

Commit 1fb77418e7d511a557ffdb34f0f5a985c6470b6b

Key changes:

  • The data field in the Func, ImportObjectBuilder, and PluginModuleBuilder structs has been changed from Option<Box<T>> to Option<&mut T>.
  • The Function::create_sync_func method in the Func and ImportObjectBuilder structs has been updated to reflect the new data field type.

Potential problems:

  • The change from Option<Box<T>> to Option<&mut T> may introduce mutability issues. It's important to ensure that the new usage of &mut does not lead to mutable aliasing or data races.
  • The impact of this change on existing code is unclear without further context. It's important to review the changes in the context of the entire codebase and consider any potential breaking changes or conflicts with other code.
  • It's unclear why these changes were made and if they are necessary for the functionality of the code. It would be helpful to have documentation or comments explaining the reasoning behind these changes.

Copy link
Collaborator

@L-jasmine L-jasmine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using Option<&mut T> to identify func's data as unsafe is because the lifetime of T is not bound to the lifetime of the Function, and T might be moved after creating the Function.

Copy link
Collaborator

@L-jasmine L-jasmine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix it as soon as possible in the upcoming updates.

@apepkuss
Copy link
Collaborator Author

Using Option<&mut T> to identify func's data as unsafe is because the lifetime of T is not bound to the lifetime of the Function, and T might be moved after creating the Function.

#27 tracks the issue. We'll fix it in another PR. Thanks a lot!

@apepkuss
Copy link
Collaborator Author

@L-jasmine Thanks for the review!

@apepkuss apepkuss merged commit ee2f127 into WasmEdge:main Jul 21, 2023
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.

3 participants