Rename `grow_memory` and `memory_size`?
binji opened this issue · 24 comments
--bikeshed warning--
As discussed here, we will likely add a suite of new memory operations, mem.init, mem.clear, mem.copy, etc.
We could follow the current naming scheme and call these init_memory, clear_memory, copy_memory (or move_memory), but the mem. syntax is kind of nice, and mimics the style of other instructions. But then we have a bit of a naming wart with grow_memory and memory_size. Should we rename these to mem.grow and mem.size?
I like mem.grow and mem.size better, and I think it's more consistent with our other names, like i32.add.
Will many things break if we change it?
The names are referenced in the spec, and exposed to users in the text format. Browser's debuggers display these names too.
But I'm guessing most things won't break because most users and tools don't consume or produce the text format.
While we're here: mem versus memory?
I prefer mem, but memory is fine too.
+1. If we do this before 1.0 is official we can get away with it.
FWIW, I also prefer mem.
What have we shortened in the spec, versus spelled out fully? I think we should be consistent and choose mem versus memory accordingly.
@jfbastien, at least the core spec abbreviates to "mem" everywhere else.
In long names we have Memory, Table, Global, reinterpret, call_indirect, etc.
In shortened names we have varuint32, i32.const, eq / ne / lt / etc, clz, mul, div, rem, trunc.
So we've been quite inconsistent! I don't care which way we go, but we should be consistent. We spell it "Memory" in the current spec, I'd expect memory operations to be spelled the same, but we could decide that all ops need to be abbreviated while object need to be spelled out fully?
This is the tiniest of bikeshed anyways, I don't really care which way we go, but consistency seems missing and that's kind of embarrassing for a Standard (granted, we just accreted things over time and are just stamping them into the standard!).
In the context of assembly instructions,
memshould be perfectly
recognizable, so it seems like going withmemovermemoryfollows the
general rule of using abbreviations where they are well known.
Sure, but the text format already uses memory. Granted it's not an instruction. I think we shouldn't be inconsistent.
These names are also exposed to users in various tools; clang has an intrinsic function named __builtin_wasm_grow_memory, LLVM has @llvm.wasm.grow.memory, and Rust and others will likely expose them as well. These tools don't have to reflect the official names, but it's advantageous if they do. So there is value in resolving this sooner rather than later.
The only outstanding issue here is memory vs mem, with most people preferring mem. The arguments against mem are:
- consistency with "Memory" in the JS API
- consistency with the text format already using "memory"
- "memory" is what the current instructions use.
As a way to move forward, I propose observing:
- the text format already has
func, a short name, in a similar role asmem(both have corresponding module index spaces), so there's consistency to be had there - the rename is a text format change, so other text format changes may be considered at the same time
- the original names are being replaced, so they're not great examples for new names :-)
So the changes can be:
mem.growandmem.size.- rename
(memory ...)to(mem ...)in the text format to match - tools may continue to accept the old names, as needed, for compatibility
How does this sound?
I just have a question from the point of view of an implementer; unlike WASM, WATF has no version or header. Wouldn't it be worth adding something like (version 1.0) (module) to allow future breaking changes?
During the last meeting some participants did not like the shortening in the memory declaration form. After having converted the entire test suite in #649, I tend to agree, for both aesthetics and minimality of change. I created #720 as an alternative PR that picks the longer names memory.size and memory.grow. We should decide at the next CG meeting.
I thought the concern was more about the memory section being too short if we switched it to mem; weren't we discussing the possibility of instructions going by mem.grow, etc, and the section staying memory?
@littledan, there was a strong sentiment in the above discussion that both should be consistent, and I agree.
As a random aside - if memory.grow is preferable to grow_memory, surely also global.get and local.tee are preferable to get_global and tee_local? They operate on global/local objects, in the same way that the memory opcodes operate on a memory object.
@NWilson, funny you bring that up, because I was just editing a comment to the same effect. Right now, it is not really clear what our overarching naming scheme for instructions is. With the proposed change, we have X. prefixes where X can be one of two things: (1) a value type, (2) an entity (one of the things that has either its own namespace or can be ex/imported). If we wanted to be more consistent, then we may also want to rename the *global and *local instructions. One could even argue that call_indirect should be renamed table.call, which seems in line with the table.get/set instructions proposed elsewhere.
I don't have a good answer. It becomes even less clear when considering various future extensions.
This seems like the best possible time to go over the MVP spec and all the upcoming extensions that have proposed their own new opcodes to come up with some kind of harmony and consistency.
At the March 21st CG meeting, we discussed whether to choose memory or mem. There weren't any strong opinions about it w.r.t. the instruction names, but some preferred that the section name not be shortened to mem, and there was an additional desire to be consistent between the instruction and section names. As a result, the group decided to go with memory.
This has landed.