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 PluginModule and WasiModule in wasmedge-sys #42

Merged
merged 9 commits into from
Jul 28, 2023

Conversation

apepkuss
Copy link
Collaborator

In this PR, PluginModule and WasiModule in wasmedge-sys are refactored. In addition, the relevant APIs are updated.

Copy link
Member

juntao commented Jul 28, 2023

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


Overall Summary:

The Pull Request titled "[Refactor] Update PluginModule and WasiModule in wasmedge-sys" contains several individual code patches, each with its own set of changes and potential problems.

Some of the potential issues and errors found in the individual patches include:

  • In the first patch, the changes made to the WasiModule struct raise concerns about the removed methods and traits. It's unclear if the necessary functionality has been adequately addressed, and compatibility issues with existing code that relies on these traits may arise.

  • The changes in the PluginModule struct, particularly the removal of the generic type parameter and the host_data field, may have compatibility implications and could impact code that relied on these features.

  • The removal of the AsInstance trait implementation in the WasiInstance struct may impact other parts of the code that depend on this trait.

  • The changes in the PluginModuleBuilder struct and related types do not raise any potential problems.

  • The patch involving the addition of the new module wasi lacks clear documentation and explanations for the changes made. The purpose and relation to the overall codebase are unclear.

  • The patch addressing a clippy issue does not raise any potential problems.

  • The patch renaming ImportObject to WasiInstance lacks documentation and explanations for the change. The patch seems incomplete and does not provide any code review comments or suggestions.

  • The patch involving the renaming of method and parameter names in the Executor struct raises concerns about compatibility and lacks clear explanations for the rename.

In summary, the Pull Request includes various changes with some potential issues and errors. It is recommended to address the concerns raised and provide more detailed documentation and explanations for the changes made. Code review comments and suggestions should also be included for a comprehensive review.

Details

Commit 3edcc8e5a15a1e3f73f3774be6ad60e83db87cea

Key Changes:

  • Removed several methods for retrieving exported function, table, memory, and global instances from the WasiModule struct.
  • Added a name method to retrieve the name of the module instance.
  • Removed the AsImport and AsInstance traits implementation for the WasiModule struct, which included implementation details for adding functions, tables, and their associated names to the module.

Potential Problems:

  • It seems that the removed methods for retrieving exported instances have been merged into the AsInstance trait implementation, so it's unclear if the logic behind these methods has been adequately addressed. It would be helpful to review the changes in the AsInstance trait implementation to ensure that the required functionality is still provided.
  • The removal of the AsImport and AsInstance traits implementations raises concerns about how those features are implemented now and if there could potentially be compatibility issues with existing code that relies on these traits.
  • Overall, a more comprehensive commit message or PR description explaining the motivation behind these changes would be beneficial in understanding the changes made.

Commit 1dc2c2964b385b4ab2662ed6320a64270e4a5985

Key changes:

  • The PluginModule struct no longer has a generic type parameter.
  • The _host_data field has been removed from the PluginModule struct.
  • The create method of the PluginModule struct now accepts a boxed host context data host_data directly instead of an Option<Box<T>>, where T is the host data type.
  • The host_data field has been removed from the PluginModule struct.
  • The funcs field of the PluginModule struct is now initialized as an empty vector.

Potential problems:

  • The generic type parameter T has been removed from the PluginModule struct. This may have compatibility implications if the codebase relied on PluginModule being generic.
  • The host_data field is no longer stored in the PluginModule struct. If this data is required elsewhere in the code, it may need to be accessed through another means.
  • The host_data type parameter is not used in the create function signature. This may be an oversight and should be addressed if necessary.
  • It is unclear if the removal of T as a generic type parameter impacts other parts of the codebase that interact with PluginModule. This should be reviewed and resolved if any issues are found.

Commit 2a1e92ac07bfc5940918d255a3f1d16d39aba3ed

Key changes in the patch:

  • The cfg_if block has been removed and the use statements for various types have been removed from the code. This indicates that the code is no longer using those types and features.
  • The WasiInstance struct has been moved out of the conditional compilation block and is now unconditionally defined.
  • The AsInstance trait implementation for WasiInstance has been removed.
  • The WasiInstance implementation now includes a name method that returns the name of the instance.
  • Several other methods for retrieving exported functions, globals, memories, and tables have been removed from the WasiInstance struct.

Potential problems:

  • The removed cfg_if block and use statements might indicate that some functionality has been removed or modified in the codebase. Without further context, it is not clear why these changes were made and if they will cause any issues.
  • The removal of the AsInstance trait implementation might impact other parts of the code that rely on this trait. It would be important to check if any other code depends on this implementation and make appropriate adjustments if necessary.

Commit 6ae1df89fca3e27eb6eff25f74418b2b20e6546b

Key changes:

  • The struct PluginModuleBuilder<T> has been updated to allow for trait objects with the ?Sized keyword.
  • The build method of PluginModuleBuilder<T> now returns a WasmEdgeResult<PluginModule> instead of WasmEdgeResult<PluginModule<T>>.
  • The struct PluginModule<T> has been updated to remove the type parameter T.

Potential problems:

  • There doesn't seem to be any potential problems with the changes made in this patch.

Commit c9b4d40e04ac7d4451353001d1ce080dfe58cc18

Key Changes:

  • Added a new module wasi in lib.rs when the async feature is not enabled.

Potential Problems:

  • The commit message is not clear and does not provide sufficient information about the changes made in the patch. It should clearly explain the purpose of the change.
  • The changes in lib.rs are not clearly explained. It is unclear why the new module wasi is added only when the async feature is not enabled. This change should be documented in a comment or commit message to provide context.
  • The changes in vm.rs seem to be unrelated to the commit message. It is unclear why these changes are made and how they are related to updating the code.

Overall, the patch contains unclear changes and lacks sufficient documentation, making it difficult to review and evaluate the impact of the changes. It is recommended to provide more detailed explanations of the changes made and their purpose.

Commit 8fcd4668d7f7e4e69952ac97f254f59e6caf0d45

Key changes:

  • Fixes a clippy issue in the plugin module.

Potential problems:

  • None identified.

Commit 1faa7bc7a66cbb8f3823daf0c1e59a9072930f3b

Key changes:

  • Renamed ImportObject to WasiInstance.

Potential problems:

  • The patch seems to be incomplete as it only includes changes to three files. It's possible that more changes were made but not included in the patch.
  • It's not clear why the ImportObject was renamed to WasiInstance. There is no explanation provided in the patch. It would be helpful to have a comment or commit message explaining the reasoning behind the change.
  • Apart from the renaming, there doesn't seem to be any other significant changes in this patch. It's possible that other changes were made in previous patches that are not included here.
  • No code review comments or suggestions are provided in this patch.

Commit ae1afe1fdcba2190891a50495dcc82f07edeaa8e

Key changes in the patch:

  • The method register_import_object in the Executor struct has been renamed to register_wasi_instance.
  • The parameter name import in the register_wasi_instance method has been changed to instance.
  • The import object types have been renamed from ImportObject::Wasi to WasiInstance::Wasi and from ImportObject::AsyncWasi to WasiInstance::AsyncWasi.

Potential problems:

  • It is unclear why the method and parameter names have been changed. The commit message does not provide a clear rationale for the rename.
  • It is possible that this change could introduce compatibility issues with existing code that relies on the register_import_object method name.
  • The patch does not provide any information about the reasoning behind the import object type name changes. It would be helpful to have more context on why these changes were made.

Commit 32c266e2353a01babe68dce590122d8b1e1cb5d6

Key changes:

  • The register_import_object method calls have been replaced with calls to the register_wasi_instance method.
  • The sys::ImportObject::Wasi and sys::ImportObject::AsyncWasi values have been replaced with sys::WasiInstance::Wasi and sys::WasiInstance::AsyncWasi, respectively.

Potential problems:

  • None found.

Overall, this patch refactors the VmBuilder::build method in the src/vm.rs file to use the updated methods and types from the wasmedge-sys library. The changes appear to be correct and shouldn't introduce any problems.

@apepkuss apepkuss requested a review from L-jasmine July 28, 2023 06:04
@apepkuss apepkuss merged commit 379bc7a into WasmEdge:main Jul 28, 2023
16 of 19 checks passed
@apepkuss apepkuss deleted the refactor/update-plugin-module branch July 28, 2023 07:05
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