-
Notifications
You must be signed in to change notification settings - Fork 21
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
Rename Index Type
to something else, perhaps Offset Type
#67
Comments
What's wrong with "index type"? I haven't been following #50 very closely, but I struggle to think of a better term generally. If it were only memories, you could call it an "address type", but since tables are in the mix I think "index" is the better general term. |
Context: #50 (comment) |
If I'm understanding correctly, the spec today currently doesn't choose an official name like "index" for the parameter to Anyway, unless there's something I'm missing, I think "index type" is a perfectly good term that I'd vote to keep. |
I'm basically neutral on this. I've gotten used to calling it index type, so there is some inertia there I suppose. |
@bvisnuss, I'd prefer avoiding overloading terminology in potentially confusing ways. We already use the term "index" for all the index spaces, and in particular, "memory index" and "table index" already have a different meaning. Moreover, while "index" is natural for tables, it is less so for memories. Hence I suggested "offset" as a more neutral and unclobbered name, but perhaps there are better suggestions. |
I agree that "index" is the most natural term in isolation, but I do agree that avoiding overloading it is a good idea. I would prefer "address" over "offset," even for tables, and it would be an even less overloaded term. It would be weird to explain the offsets in memargs as being "offsets from an address/index that has an offset type." |
Address type (addrtype) sounds good to me. |
I've been making a variety of spec updates in #71, and would be happy to change "index type" to "address type" as part of that PR if people agree. Any objections? |
@bvisness just FYI I've been doing all my recent spec work on the wasm-3.0 branch of this fork, not the main branch. This is so that all our work here can be merged into the wasm-3.0 branch in the upstream spec repo. |
I'm working on a PR for this issue. This is an almost invisible change to users except for one thing: the Presumably this parameter should be renamed. Should it be renamed to |
This was discussed in WebAssembly/memory64#67 and changed in WebAssembly/memory64#90. This is a pure mechanical renaming of "IndexType" -> "AddressType" and "index_type" -> "address_type". [email protected] Bug: 41480462 Change-Id: Ifb6843ca47d9a48d7aac3d7f64d0f7c7ce4ecec5 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5941627 Commit-Queue: Clemens Backes <[email protected]> Reviewed-by: Eva Herencsárová <[email protected]> Cr-Commit-Position: refs/heads/main@{#96704}
No description provided.
The text was updated successfully, but these errors were encountered: