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 proc-macros in wasmedge-macro #49

Merged

Conversation

apepkuss
Copy link
Collaborator

@apepkuss apepkuss commented Aug 4, 2023

In this PR, the host_function and async_host_function proc-macros in wasmedge-macro are improved. In addition, introduce a new API named generate in WasiContext of wasmedge-sdk.

Copy link
Member

juntao commented Aug 4, 2023

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


Overall Summary:

In general, the pull request titled "[Refactor] Update proc-macros in wasmedge-macro" seems to be a mostly positive set of changes. However, there are some potential issues and errors that should be addressed.

The first set of changes focuses on refactoring the host_function and async_host_function proc-macros. While the code has been rearranged to improve readability, there are a few potential problems. The error handling is currently not well-defined and could be improved by returning an error result or providing a helpful error message instead of panicking. Additionally, there is duplicated code in the expand_host_func_with_two_args and expand_host_func_with_three_args functions, which could be merged to reduce redundancy. Finally, the code could benefit from additional comments to clarify certain sections and variables.

The second set of changes updates the version of rust-macro to 0.6.1. No potential problems were identified in this patch.

The third set of changes adds a new API WasiContext::generate to create a WasiContext with specific arguments, environment variables, and preopened directories. No potential problems were found in this patch.

The final set of changes bumps the version of wasmedge-sdk to 0.11.1 in the Cargo.toml file. No potential problems were found in this patch.

To summarize, the refactoring changes in the first set have potential issues and errors that should be addressed. The other sets of changes seem fine without any identified problems.

Details

Commit 7d13b92fb30a86cf79cdc45b67f41504097a6a4f

Key changes:

  • The host_function and async_host_function proc-macros in wasmedge-macro have been refactored.
  • The function expand_host_func has been updated to handle two different numbers of arguments (2 and 3).
  • The code has been rearranged to improve readability.
  • Changes have been made to extract the arguments and handle different types of arguments.

Potential problems:

  • The error handling is not well-defined. It currently panics with a message for invalid numbers of host function arguments. It would be better to handle this in a more graceful way, such as returning an error result or providing a helpful error message.
  • It is not clear what the purpose of the expand_host_func_with_two_args and expand_host_func_with_three_args functions is. It seems they could be merged into a single function, as they have a lot of duplicated code.
  • The code could benefit from additional comments to clarify the purpose of certain sections and variables.

Commit b693f8807d3ef5f817bcf9a16754b2dcca24b1e0

Key Changes:

  • The version of rust-macro in wasmedge-macro has been bumped to 0.6.1.

Potential Problems:

  • No potential problems have been identified in this patch.

Commit 2a4244099d5074e09f5f8b8c702f7d708fb9a037

Key changes:

  • Added a new API WasiContext::generate to create a WasiContext with specific arguments, environment variables, and preopened directories.

Potential problems:

  • None apparent. The code looks fine.

Commit c034a9ff9f7b4ec301eaed4524aa7b320324db3a

Key changes:

  • Bumped the version of the wasmedge-sdk to 0.11.1 in the Cargo.toml file.

Potential problems:

  • No potential problems were found.

@apepkuss
Copy link
Collaborator Author

apepkuss commented Aug 4, 2023

@L-jasmine Thanks for the review!

@apepkuss apepkuss merged commit d178953 into WasmEdge:main Aug 4, 2023
16 of 19 checks passed
@apepkuss apepkuss deleted the refactor/update-procmacro-async-host-func branch August 4, 2023 08:35
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