Skip to content

Removed a few #[allow(dead_code)] #4299

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

Merged
merged 3 commits into from
Nov 29, 2024

Conversation

RunDevelopment
Copy link
Contributor

Changes:

  • Removed unused nargs field in AuxImport::Closure. The field wasn't used because the generated JS code used ...args to capture any number of arguments. If this information is necessary in the future, AuxImport::Closure already includes the adapter ID of the closure function and the number of args can be retrieved from that.
  • Removed unused hole field in Instruction::OptionWasmToStringEnum. The bindings have a comment explaining how the hole is handled implicitly as an "invalid" value, so it doesn't need to know the actual value of the hole.
  • Changed how 8/16/32/64 bit ints are converted.

So about the integer conversion: Instead of removing the unused fields, I realized that Instruction::IntToWasm and Instruction::WasmToInt were kind of busted. The main issue is that they not only carried redundant data, but could also carry invalid data.

To fix this, I first split both into 64 and 32 variants. This immediately makes the JS -> WASM conversions nops. The WASM -> JS directions are also simpler, because they just need a single bool of data to select the correct conversion.

This also fixes a minor bug in representable_without_js_glue. The function previously incorrectly determined that WASM -> JS u64 and WASM -> JS NonNull did not require glue code. This was trivial to fix with the new representation.


This PR should not affect the output of the CLI in any way.

Verified

This commit was signed with the committer’s verified signature.
IvanGoncharov Ivan Goncharov
Copy link
Collaborator

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

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

LGTM.
Will look into the other unused fields.

@daxpedda daxpedda merged commit 32db0f4 into rustwasm:main Nov 29, 2024
46 checks passed
@RunDevelopment RunDevelopment deleted the removed-some-allow-dead branch November 29, 2024 18:25
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.

None yet

2 participants