bytecodealliance / bytecodealliance/wasmtime
Cranelift: rename `PrimaryMap` and `SecondaryMap` to `FixedSizeVec` and `AutoResizeVec`
- Dominant language
- Rust
- Stars
- 18.6k
- Forks
- 1.8k
- Avg merge
- 1d 19h
- Merged PRs (30d)
- 121
Description
The names `PrimaryMap` and `SecondaryMap` are misleading and un-descriptive. For the benefit of readers and maintainers of Cranelift, and especially newcomers, I propose we give them better names.
* The `Map` component is misleading. Neither type possesses what I think of as the primary characteristic of a map (in the sense of Rust `HashMap`, libstdc++ `map`, etc), which is O(1 + epsilon) access time for sparse keys. The new names shouldn't include the text `Map`.
* These types are in fact vectors, and you need to know that to use them reasonably (meaning, they only work well for dense zero-based key ranges) but there's nothing in the name to suggest that.
* `Primary` and `Secondary` suggest some relationship between individual maps which simply doesn't exist. This confused me for some time when I first came to the code base. I thought: how do I find all the `SecondaryMaps` associated with a `PrimaryMap`? Are they connected via some Rust type system magic? Or is it done at run time; if so is there some registry I should consult? This is of course totally irrelevant; there is no such connection.
I suggest to rename `PrimaryMap` to `FixedSizeVec` and `SecondaryMap` to `AutoResizeVec`:
* it doesn't claim they are maps, which they aren't
* it does claim they are vectors, which they are
* it doesn't claim any relationship between them, which there isn't
* it correctly summarises the essential difference between them: one is fixed size, the other resizes (upwards) on demand.
I believe that accurate naming is important, and this would be a small but important step in making Cranelift easier to understand. If there is general support I will happily prepare a patch.
Contributor guide
Assessment
This issue has not been assessed yet.