fix(HashMap): change capacity() to *const Self
#19770
Conversation
There was a problem hiding this comment.
This PR does raise the question of why we are passing by const pointer so much in hash_map.zig. In HashMapUnmanaged, .capacity() requires a const pointer because .header() requires a const pointer, but looking at .header(), there is nothing that we need a pointer to the hash map for. Replacing these parameters with just the value, all the tests pass. Maybe I'm missing something, but I can't see why we aren't doing this. Does anyone have a good reason for passing by pointer?
|
The only reason for using pointers instead of just the value is to force the hashmap to be passed by reference instead of relying on the compiler's implicit pass-by-reference. That being said, the compiler should only turn value parameters into reference parameters when it's faster. By explicitly having the parameter be a const pointer, we lose out on this potential optimization. If this was done originally because the compiler was choosing to pass the hashmap by value but it was actually faster to pass by reference then that sounds like a compiler bug and we should fix that instead of using a workaround. The standard library is the last place I would expect to see compiler bug workarounds. |
|
The above analysis is accurate -- such workarounds are effectively banned in OP: your change here is completely fine, and I'll be happy to merge it as-is, but would you be willing to go the extra mile and look over the existing uses of |
|
yeah sure i can checkout the additional changes |
|
Please don't open a PR without testing the code first. It doesn't even compile. |
|
the original pr compiled fine, which is why i opened it - these changes weren't complete yet because i was having troubles compiling from source/testing |
- Used `Self` instead of `*const Self` where appropriate (orignally proposed in ziglang#19770) - Replaced `@intFromPtr` and `@ptrFromInt` with `@ptrCast`, `@alignCast`, and pointer arithmetic where appropriate With this, the only remaining instance on pointer-int conversion in hash_map.zig is in `HashMapUnmanaged.removeByPtr`, which easily be able to be eliminated once pointer subtraction is supported.
HashMap.capacityrequires a mutable reference whereas it should be const