bytecodealliance / bytecodealliance/wasmtime

Cranelift: rename `PrimaryMap` and `SecondaryMap` to `FixedSizeVec` and `AutoResizeVec`

Open
#1,583 6 comments 0 reactions 0 assignees View on GitHub
cranelift
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.