Add Hash#transform_keys!#16280
Conversation
370f0d6 to
f79ec79
Compare
| initialize_dup_entries(transformed_hash) # we need only to copy the buffer | ||
| initialize_copy_non_entries_vars(transformed_hash) | ||
| self | ||
| end |
There was a problem hiding this comment.
It probably makes sense to move the copying logic into a new method #replace, similar to Hash#replace in Ruby.
|
Does it make sense to update |
|
Yeah, sounds good to update |
|
@straight-shoota Updated the |
src/hash.cr
Outdated
| # hash # => {"a" => 1, "bb" => 2, "ccc" => 3} | ||
| # ``` | ||
| def transform_keys!(&block : K, V -> K) : self | ||
| transformed_hash = self.transform_keys(&block) |
There was a problem hiding this comment.
Presumably the block forwarding was chosen here because the yielding form wouldn't compile due to a mismatch between K2 and K. In this case I think it is acceptable to just inline #transform_keys's body here, since captured blocks have a non-negligible runtime cost:
| transformed_hash = self.transform_keys(&block) | |
| transformed_hash = Hash(K, V).new(initial_capacity: entries_capacity) | |
| each_with_object(transformed_hash) do |(key, value), memo| | |
| memo[yield(key, value)] = value | |
| end |
And then Set#map! could also use the yielding form.
There was a problem hiding this comment.
If you mean by "yielding form" this approach:
transformed_hash = self.transform_keys { |k, v| yield k, v }then yes, it doesn't compile:
bin/crystal spec/std/hash_spec.cr
Using compiled compiler at .build/crystal
Showing last frame. Use --error-trace for full trace.
In src/pointer.cr:146:29
146 | (self + offset).value = value
^----
Error: type must be Hash::Entry(Int32 | String, String), not Hash::Entry(Int32, String)
There was a problem hiding this comment.
Done. Thank you!
There was a problem hiding this comment.
I believe we should be fine with an explicit cast to K. No need to duplicate the implementation.
transformed_hash = transform_keys{ |k, v| (yield k, v).as(K) }Sorry for the back and forth Andrii, and thanks for staying around with us 🫶
Changes:
Hash#transform_keys!Related discussion - #16271 (comment).