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

Spec test updates #91

Merged
merged 7 commits into from
Oct 22, 2024
Merged

Spec test updates #91

merged 7 commits into from
Oct 22, 2024

Conversation

bvisness
Copy link
Collaborator

Adds core and JS API spec tests for recent updates as described in #80.

We needed fewer updates than I had originally thought. See the individual commits. Tests I did not have to write include:

Beyond adding tests for memory64, I also took the opportunity to significantly enhance the memory import tests, since multi-memory is now standard.

Resolves #80.

@bvisness bvisness requested a review from sbc100 October 17, 2024 10:10
Copy link
Member

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

Fantastic, thanks for working on this Ben!

I wonder if we should split out the js-tests into a separate PR since they seem logically separate to me (and the imports.wast change includes other misc changes for multi-memory)?

test/core/imports.wast Show resolved Hide resolved
test/js-api/memory/constructor.any.js Show resolved Hide resolved
test/js-api/table/assertions.js Outdated Show resolved Hide resolved
test/js-api/table/assertions.js Outdated Show resolved Hide resolved
@sbc100 sbc100 requested a review from backes October 17, 2024 18:00
@bvisness bvisness marked this pull request as draft October 18, 2024 10:15
@bvisness
Copy link
Collaborator Author

Converting to draft while I update the spec tests to reflect the change to "address type".

📣 ATTENTION! 📣 This is your last chance to bikeshed the name of the parameter used in memory and table descriptors! It used to be "index", but I changed it to "addressType" in #90 after discussion in #67. If you would prefer it to be "address" or something else, please let me know now so I can update the spec and tests! (cc @sbc100, @backes, @rossberg, and @tlively, who all participated in #67 but did not express an opinion there.)

@backes
Copy link
Member

backes commented Oct 18, 2024

"addressType" sounds good to me. This PR is not updated for that yet, right?

@bvisness
Copy link
Collaborator Author

@eqrion has pointed out to me that WebAssembly.Global uses "value" for the global's value type, and WebAssembly.Table uses "element" for the element type. Given that precedent, I'm now strongly feeling like it should be "address" instead of "addressType". Sorry I didn't think of this sooner, but hopefully it's no trouble for people to support once I get it in the spec.

@sbc100
Copy link
Member

sbc100 commented Oct 18, 2024

@eqrion has pointed out to me that WebAssembly.Global uses "value" for the global's value type, and WebAssembly.Table uses "element" for the element type. Given that precedent, I'm now strongly feeling like it should be "address" instead of "addressType". Sorry I didn't think of this sooner, but hopefully it's no trouble for people to support once I get it in the spec.

addressType seems like the most explicit. Its more verbose but probably better to be explicit, right?

@tlively
Copy link
Member

tlively commented Oct 18, 2024

No strong preference between address and addressType. addressType seems nicer in isolation, but I agree that it is good to keep the naming scheme for similar properties consistent across different parts of the API.

@eqrion
Copy link
Contributor

eqrion commented Oct 18, 2024

I would also prefer addressType if it weren't for the fact that the JS-API already has a strong precedent for using the shorter name. I personally think it would have been better for globals to take valueType instead of value, and ditto for tables. But they don't so I think we should use address to match them.

@bvisness bvisness marked this pull request as ready for review October 22, 2024 14:53
@bvisness
Copy link
Collaborator Author

Given the prior positive reviews, and no dissent on the change to address, I'm going to go ahead and merge this.

@bvisness bvisness merged commit ade7380 into WebAssembly:main Oct 22, 2024
1 check passed
hubot pushed a commit to v8/v8 that referenced this pull request Oct 28, 2024
Import upstream spec tests, in particular including
WebAssembly/memory64#91 and
WebAssembly/memory64#92.

[email protected]

Bug: 364917766
Change-Id: I0dd2afab787e1257ee018fdf214bb9173c414572
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5968220
Reviewed-by: Eva Herencsárová <[email protected]>
Commit-Queue: Clemens Backes <[email protected]>
Cr-Commit-Position: refs/heads/main@{#96851}
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.

Tests needed for recent spec updates
6 participants