Rename anyref to externref to match proposal change #2900
Conversation
scripts/test/shared.py
Outdated
| @@ -229,7 +229,7 @@ def has_shell_timeout(): | |||
| '--wasm-staging', | |||
| '--experimental-wasm-eh', | |||
| '--experimental-wasm-simd', | |||
| '--experimental-wasm-anyref', | |||
| '--experimental-wasm-externref', | |||
There was a problem hiding this comment.
Should we use this opportunity to also change this to --experimental-wasm-reftypes` to match browser flags since I think the rest of the proposal is implemented too under the same flag?
jayphelps
force-pushed
the
externref
branch
2 times, most recently
from
2fd7464 to
5bb1c9c
Compare
|
The other major change here is to require a type parameter for |
|
@binji wasn't the discussion today about removing that type param for ref.null? I was multitasking so I may have misunderstood. |
| ;; t <: anyref for all reftypes t | ||
| ;; nullref <: anyref, nullref <: funcref and nullref <: exnref | ||
| ;; t <: externref for all reftypes t | ||
| ;; nullref <: externref, nullref <: funcref and nullref <: exnref |
There was a problem hiding this comment.
The actual subtyping relationship will have to be updated, but I think that can be a separate PR as well. Can you add a TODO here?
There was a problem hiding this comment.
Oh yikes. Indeed. I digged a bit and it touches area of the codebase I'm not comfortable touching at the moment, e.g. fuzzer, so I'll indeed comment this as a TODO comment. Thanks!
No, that was about removing type type immediate for |
|
The emscripten tests are failing but the errors are unclear to me and I have to pause on this until this evening. If someone happens to have a second to grok what the issue is it'd be appreciated https://github.com/WebAssembly/binaryen/pull/2900/checks?check_run_id=755283093 |
|
Hmm.. looks like maybe non-determinism in the validator causing the tests to produce different output between runs maybe? I can't see that its related to this PR in any way, can you? |
|
Re-running the CI to see its a flake. |
You deleted a trailing whitespace in test/binaryen.js/kitchen-sink.js.txt :) |
jayphelps
force-pushed
the
externref
branch
3 times, most recently
from
3f7966b to
6a5b74f
Compare
|
Phew! OK all green now. The issue was that search and replace in vscode applies default style rules too, which include no trailing whitespace. There is a single space after I think this is ready for final review/merge |
There was a problem hiding this comment.
It looks good to merge, modulo the super minor nit I just posted here.
Unrelated question: Are you also planning to do update Binaryen codebase on other subtyping-related changes (new instructions and such)? That'd be also appreciated; I'm just asking in order not to do the duplicate work from my side :)
|
Thanks! One more thing, force-pushing makes reviewing only addition changes harder. If possible, please just add additional commits! :) (You don't need to rebase all commits into one each time, because we are gonna squash all commits into a single commit when merging this PR anyway) |
|
@aheejin will do! |
|
@aheejin Unfortunately it's unlikely I'll have time right now, as this rename was the only change I needed to unblock my work. I looked a bit into the other changes needed and they touch parts of the codebase I'm not yet familiar with (e.g. fuzzing code) and I can't spare the cycles at the moment to fully grok it and any potential fallout implications. I wish though! This stuff is a lot of fun. |
anyref future semantics were changed to only represent opaque host values, and thus renamed to externref.
Chromium was just updated to today (not yet released). I couldn't find a Mozilla bugzilla ticket mentioning externref so I don't immediately know if they've updated yet.
WebAssembly/reference-types#87