X Tutup
Skip to content

Rename internal fields and variables in AHashMap, HashMap and HashSet#107045

Merged
Repiteo merged 1 commit intogodotengine:masterfrom
Ivorforce:rename-hashing-variables
Sep 18, 2025
Merged

Rename internal fields and variables in AHashMap, HashMap and HashSet#107045
Repiteo merged 1 commit intogodotengine:masterfrom
Ivorforce:rename-hashing-variables

Conversation

@Ivorforce
Copy link
Member

@Ivorforce Ivorforce commented Jun 2, 2025

AHashMap, HashMap and HashSet have inconsistent field and variable names - not only between themselves, but also internally, and across the codebase.
When I personally tried to understand the code, this threw me off many times, and made the classes extremely hard to understand and compare.

I've renamed variables to be consistent. This should help these classes be more maintainable.

Change List

HashMapData name: AHashMap uses (globally declared) HashMapData to store hash and element_idx. However, the name makes it sounds like it stores data.
Solution: I've renamed the field to AHashMap::Metadata. This is consistent with naming terminology of hash maps in other libraries.

AHashMap::Metadata is a struct of a union of a struct or uint64_t: This is very confusing.
Solution: I've changed Metadata to be a struct, and whenever both elements are assigned at once, a Metadata instance is used, instead of assigning .data union. I tested performance (get, get missing, insert, insert existing, erase), and found no regressions (if anything, it's 1-2% faster now).

Indexes are inconsistently named: Indexes are named pos, index, idx, kpos, hpos, among others. Further, in AHashMap and HashSet, pos may refer to either key or hash index inconsistently, even within the same function. This makes it extremely difficult to know at a glance which kind of index you're looking at.
Solution: HashMap has only one kind of index. All indices are called idx. AHashMap has elements and metadata, so it now uses metadata_idx and element_idx. HashSet has hashes and keys, so it now uses hash_idx and key_idx. capacity_idx is now consistently named capacity_idx.

Fields are indistinguishable from local variables: For example, AHashMap has local capacity and the field capacity.
Solution: Private fields are prefixed with underscore _ to mark them as such.

capacity is indistinguishable from capacity mask: AHashMap has field capacity, but it's actually capacity - 1.
Solution: I've renamed the field to _capacity_mask.

num_elements is inconsistent with size of other containers: Other collections like Array use size() to refer to the number of elements in the collection. The public APIs of hash maps also use size(), but the internal field is called num_elements.
Solution: num_elements is renamed to size for consistency.

There are other, smaller changes to pull everything together, but I think this covers the big ones.

@Ivorforce Ivorforce added this to the 4.x milestone Jun 2, 2025
@Ivorforce Ivorforce requested a review from a team as a code owner June 2, 2025 12:13
@Ivorforce Ivorforce force-pushed the rename-hashing-variables branch 5 times, most recently from eb472bd to 7cd8c6b Compare June 4, 2025 19:18
Copy link
Member

@lawnjelly lawnjelly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like straight forward renaming, and the reasoning seems sound in the OP.
Caveat I'm not super familiar with these hash maps (it's been a while since I looked at any of them).

@Repiteo Repiteo modified the milestones: 4.x, 4.6 Jul 1, 2025
@Repiteo
Copy link
Contributor

Repiteo commented Sep 17, 2025

Needs a rebase to account for #108836

@Ivorforce Ivorforce force-pushed the rename-hashing-variables branch from 7cd8c6b to 46b88dc Compare September 17, 2025 17:18
@Repiteo Repiteo merged commit aad046e into godotengine:master Sep 18, 2025
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Sep 18, 2025

Thanks!

@Ivorforce Ivorforce deleted the rename-hashing-variables branch September 18, 2025 17:53
@nikitalita
Copy link
Contributor

nikitalita commented Sep 25, 2025

This broke all the debug visualizers. godot.natvis, the gdb scripts, etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

X Tutup