Skip to content

Add initial (binary only) support for compact imports proposal. NFC.#8226

Merged
sbc100 merged 1 commit into
mainfrom
compact_imports
Jun 10, 2026
Merged

Add initial (binary only) support for compact imports proposal. NFC.#8226
sbc100 merged 1 commit into
mainfrom
compact_imports

Conversation

@sbc100

@sbc100 sbc100 commented Jan 22, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

See https://github.com/WebAssembly/compact-import-section

This change only adds binary support. Luckily the upstream tests are devided in two seperate
files binary-compact-imports.wast and compact-imports.wast so this change only imports
one of them.

@sbc100 sbc100 force-pushed the compact_imports branch 5 times, most recently from c48fb7e to 9248800 Compare January 23, 2026 21:34
@sbc100 sbc100 requested a review from a team as a code owner March 23, 2026 23:08
@sbc100 sbc100 requested review from stevenfontanella and removed request for a team March 23, 2026 23:08
@sbc100 sbc100 changed the title [WIP] Initial support for compact imports proposal. NFC. Add initial support for compact imports proposal. NFC. Mar 23, 2026
@sbc100 sbc100 requested a review from tlively March 23, 2026 23:12
@sbc100

sbc100 commented Mar 23, 2026

Copy link
Copy Markdown
Member Author

@tlively or @stevenfontanella how about we land this and I can hand it over to one of your to do the text parse stuff which is beyond my ken.

@sbc100 sbc100 changed the title Add initial support for compact imports proposal. NFC. Add initial (binary only) support for compact imports proposal. NFC. Mar 23, 2026
@sbc100

sbc100 commented Mar 23, 2026

Copy link
Copy Markdown
Member Author

I just copied the spec test directly from the upstream repo.. is that the right way to do it, or is there some script / submodule type thing for pulling those in these days?

@stevenfontanella

stevenfontanella commented Mar 23, 2026

Copy link
Copy Markdown
Member

I just copied the spec test directly from the upstream repo.. is that the right way to do it, or is there some script / submodule type thing for pulling those in these days?

Looks like it's not in the testsuite repo yet link (I'm not sure when this would happen). Seems ok to me to copy it over for now. Maybe add a TODO to replace it with the one in the testsuite repo once it's there?

how about we land this and I can hand it over to one of your to do the text parse stuff which is beyond my ken.

I'm happy to give it a try!

stevenfontanella
stevenfontanella reviewed Mar 24, 2026
View reviewed changes
Comment thread src/wasm-binary.h Outdated
Comment thread src/wasm-features.h Outdated
@sbc100 sbc100 force-pushed the compact_imports branch 4 times, most recently from 5433c45 to 176e41b Compare March 24, 2026 00:47
tlively
tlively reviewed Mar 25, 2026
View reviewed changes
Comment thread src/wasm/wasm-binary.cpp Outdated
@sbc100 sbc100 force-pushed the compact_imports branch from 458d922 to 19fced2 Compare June 9, 2026 21:54
sbc100
sbc100 commented Jun 9, 2026
View reviewed changes

@sbc100 sbc100 left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I had Gemini help me resolve the unique_ptr issue.

It chose to use a overloaded addImport along and a templatized readCompactImportsShared helper.

WDYT?

@sbc100 sbc100 force-pushed the compact_imports branch 2 times, most recently from e7477f1 to 7305451 Compare June 9, 2026 22:49
tlively
tlively reviewed Jun 10, 2026
View reviewed changes

@tlively tlively left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This seems pretty reasonable. I have a weak preference for doing an initial NFC refactoring to add all those new functions, but up to you.

Comment thread src/wasm/wasm-binary.cpp Outdated
tlively
tlively reviewed Jun 10, 2026
View reviewed changes
Comment thread src/wasm/wasm-binary.cpp Outdated
@sbc100 sbc100 mentioned this pull request Jun 10, 2026
Merged
sbc100 added a commit that referenced this pull request Jun 10, 2026
@sbc100
Refactor WasmBinaryReader::readImports to use helper methods. NFC.
9a818aa 
This change is in preparation for compact imports in #8226.
sbc100 added a commit that referenced this pull request Jun 10, 2026
@sbc100
Refactor WasmBinaryReader::readImports to use helper methods. NFC. (#…
f143d66 
…8825)

This change is in preparation for compact imports in #8226.
@sbc100 sbc100 force-pushed the compact_imports branch 3 times, most recently from 749c371 to 23db09e Compare June 10, 2026 21:29
@sbc100

sbc100 commented Jun 10, 2026

Copy link
Copy Markdown
Member Author

Rebased, and feedback addressed.

@sbc100 sbc100 requested a review from tlively June 10, 2026 21:29
tlively
tlively approved these changes Jun 10, 2026
View reviewed changes

@tlively tlively left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM % comment

Comment thread src/wasm/wasm-binary.cpp Outdated
@sbc100 sbc100 force-pushed the compact_imports branch from 23db09e to 7999756 Compare June 10, 2026 22:08
@sbc100
Add Initial support for compact imports proposal. NFC.
4674fa5 
So far this is binary-only.  We can add the text format support as
a followup I think.

https://github.com/WebAssembly/compact-import-section
@sbc100 sbc100 force-pushed the compact_imports branch from 7999756 to 4674fa5 Compare June 10, 2026 22:57
@sbc100 sbc100 enabled auto-merge (squash) June 10, 2026 23:00
@sbc100 sbc100 merged commit 7c5e27c into main Jun 10, 2026
16 checks passed
@sbc100 sbc100 deleted the compact_imports branch June 10, 2026 23:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kripken kripken kripken left review comments
@stevenfontanella stevenfontanella stevenfontanella left review comments
@tlively tlively tlively approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@sbc100 @stevenfontanella @kripken @tlively @NathanWCarlson