Stream: git-wasmtime

Topic: wasmtime / issue #1583 Cranelift: rename `PrimaryMap` and...

Wasmtime GitHub notifications bot (Oct 01 2021 at 21:11):

akirilov-arm labeled issue #1583:

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.

Last updated: Nov 22 2024 at 17:03 UTC