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

Import shared memory #519

Merged
merged 2 commits into from
Jan 9, 2025
Merged

Conversation

tonyfettes
Copy link
Contributor

Related Issues

  • Related issues: #____

Type of Pull Request

  • Bug fix
  • New feature
    • I have already discussed this feature with the maintainers.
  • Refactor
  • Performance optimization
  • Documentation
  • Other (please describe):

Does this PR change existing behavior?

  • Yes (please describe the changes below)

  • No

Does this PR introduce new dependencies?

  • No
  • Yes (please check binary size changes)

Checklist:

  • Tests added/updated for bug fixes or new features
  • Compatible with Windows/Linux/macOS

Copy link

peter-jerry-ye-code-review bot commented Dec 25, 2024

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

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

  1. Missing Newline at End of File:

    • Several files, such as README.md, moon.pkg.json, and moon.mod.json, are missing a newline at the end of the file. This is a common issue that can cause problems with version control systems and text editors. It's generally a good practice to ensure that all files end with a newline character.
  2. Inconsistent Use of shared_memory:

    • In the gen_link_command function, the shared_memory variable is used with unwrap_or(false), which defaults to false if the value is None. However, in the LinkDepItem struct, shared_memory is defined as an Option<bool>. This inconsistency could lead to confusion or bugs if the default behavior is not clearly documented or understood.
  3. Potential Typo in wasm_gc_memory_limits and wasm_gc_shared_memory:

    • In the LinkDepItem struct, the methods wasm_gc_memory_limits and wasm_gc_shared_memory incorrectly reference self.link.as_ref()?.wasm.as_ref()? instead of self.link.as_ref()?.wasm_gc.as_ref()?. This seems like a typo and could lead to incorrect behavior when accessing memory limits or shared memory settings for the wasm-gc target backend.

These issues should be reviewed and addressed to ensure the codebase remains clean and functional.

@tonyfettes tonyfettes force-pushed the import-shared-memory branch 5 times, most recently from 59485a2 to 2f63709 Compare December 25, 2024 07:29
@tonyfettes tonyfettes requested a review from Copilot December 25, 2024 07:30

Choose a reason for hiding this comment

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

Copilot reviewed 4 out of 14 changed files in this pull request and generated no comments.

Files not reviewed (10)
  • crates/moon/tests/test_cases/import_shared_memory.in/.gitignore: Language not supported
  • crates/moon/tests/test_cases/import_shared_memory.in/lib/hello.mbt: Language not supported
  • crates/moon/tests/test_cases/import_shared_memory.in/lib/hello_test.mbt: Language not supported
  • crates/moon/tests/test_cases/import_shared_memory.in/lib/moon.pkg.json: Language not supported
  • crates/moon/tests/test_cases/import_shared_memory.in/main/main.mbt: Language not supported
  • crates/moon/tests/test_cases/import_shared_memory.in/main/moon.pkg.json: Language not supported
  • crates/moon/tests/test_cases/import_shared_memory.in/moon.mod.json: Language not supported
  • crates/moonbuild/template/pkg.schema.json: Language not supported
  • docs/manual-zh/src/source/pkg_json_schema.html: Language not supported
  • docs/manual/src/source/pkg_json_schema.html: Language not supported
@tonyfettes tonyfettes marked this pull request as ready for review December 25, 2024 07:39
@tonyfettes tonyfettes force-pushed the import-shared-memory branch 2 times, most recently from 9fcbe18 to 68454a2 Compare December 26, 2024 06:50
@tonyfettes tonyfettes force-pushed the import-shared-memory branch from 68454a2 to 3579f37 Compare January 8, 2025 08:00
@tonyfettes tonyfettes requested a review from Young-Flash January 8, 2025 08:00
@Young-Flash Young-Flash merged commit a86986a into moonbitlang:main Jan 9, 2025
14 checks passed
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.

2 participants