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

WebAssembly | Table - add undocumented details and update for externref #27018

Merged
merged 41 commits into from Oct 27, 2023
Merged

WebAssembly | Table - add undocumented details and update for externref #27018

merged 41 commits into from Oct 27, 2023

Conversation

ghost
Copy link

@ghost ghost commented May 28, 2023

Description

Removes indications that WebAssembly.Table only handles functions.

Add undocumented details from the core specification that were not previous present.

Motivation

Keeping documentation up-to-date.

@github-actions github-actions bot added the Content:wasm WebAssembly docs label May 28, 2023
@ghost ghost changed the title WebAssembly | Table - propagate inclusion of externref through docs WebAssembly | Table - propagate inclusion of externref through docs and fix docs May 28, 2023
@ghost ghost changed the title WebAssembly | Table - propagate inclusion of externref through docs and fix docs WebAssembly | Table - propagate inclusion of externref through docs and add undocumented details May 28, 2023
@ghost ghost changed the title WebAssembly | Table - propagate inclusion of externref through docs and add undocumented details WebAssembly | Table - add undocumented details and update for externref May 28, 2023
@ghost ghost marked this pull request as ready for review May 28, 2023 22:14
@ghost ghost self-requested a review as a code owner May 28, 2023 22:14
@ghost ghost requested review from bsmth and removed request for a team May 28, 2023 22:14
@ghost

This comment was marked as resolved.

@github-actions github-actions bot added the merge conflicts 🚧 [PR only] label May 31, 2023
Comment on lines 31 to 33
- {{jsxref("RangeError")}}: If the current size added with `elementIncrease` exceeds the Table instance's maximum size capacity.
- {{jsxref("TypeError")}}: If `value` is not a value of the element type of the table.
- {{jsxref("RangeError")}}: If there is insufficient memory.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- {{jsxref("RangeError")}}: If the current size added with `elementIncrease` exceeds the Table instance's maximum size capacity.
- {{jsxref("TypeError")}}: If `value` is not a value of the element type of the table.
- {{jsxref("RangeError")}}: If there is insufficient memory.
- {{jsxref("RangeError")}}
- : Thrown in one of the following cases:
- If the current size added with `elementIncrease` exceeds the Table instance's maximum size capacity.
- If there is insufficient memory.
- {{jsxref("TypeError")}}
- : Thrown if `value` is not a value of the element type of the table.

...aren't the two RangeError conditions talking about the same thing, anyway?

Copy link
Author

Choose a reason for hiding this comment

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

...aren't the two RangeError conditions talking about the same thing, anyway?

No. Consider a Table that doesn't have a set maximum capacity. It's the difference between the artificial check with respect to the array bounds, versus the user agent/client running out of RAM or page memory, or anything of that sort. You can set the maximum size to (2^32 - 1), but might not be able to reach that size in reality. The maximum is just an artificial preset cap, but the memory may not necessarily be pre-allocated.

I can attempt to tersely work this into the error explanations.

Copy link
Author

Choose a reason for hiding this comment

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

@Josh-Cena Please confirm that the proceeding commit has appropriately settled any confusion regarding this.

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@ghost
Copy link
Author

ghost commented Sep 8, 2023

@bsmth @Josh-Cena It's been a while - can I get another review here?

@Josh-Cena
Copy link
Member

Since this contains non-editorial content changes, it's better to get @bsmth to look at this since I can't claim to be familiar with Wasm docs.

@ghost
Copy link
Author

ghost commented Sep 8, 2023

Ah, understood.

@github-actions github-actions bot added the merge conflicts 🚧 [PR only] label Oct 11, 2023
@github-actions
Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

@bsmth bsmth self-assigned this Oct 26, 2023
@github-actions github-actions bot removed the merge conflicts 🚧 [PR only] label Oct 27, 2023
Copy link
Member

@bsmth bsmth left a comment

Choose a reason for hiding this comment

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

Super @vimirage - I've no other suggestions, so leaving a +1 and merging shortly. Thank you 👍🏻

@bsmth bsmth merged commit a2466cc into mdn:main Oct 27, 2023
6 checks passed
@ghost ghost deleted the patch-4 branch October 27, 2023 09:23
@ghost
Copy link
Author

ghost commented Oct 27, 2023

lol I didn't use delta everywhere.

I'll do that within 24h and tag you in the PR, if you're okay with that.

@bsmth
Copy link
Member

bsmth commented Oct 27, 2023

I didn't use delta everywhere.

I'll do that within 24h and tag you in the PR, if you're okay with that.

No problem, I'm happy to have a look then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content:wasm WebAssembly docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants