Implement complex hash.#293
Conversation
|
@trotterdylan Ready for review. |
| type Complex struct { | ||
| Object | ||
| value complex128 | ||
| hash int |
There was a problem hiding this comment.
I question whether caching the hash value has much value. Note that CPython does not cache it for complex or float. Perhaps we should remove the caching in float until there's a good reason to add it.
Note that strings are good candidates for caching since a) the length of the string can be very long (amortizes the runtime of hash(s)) and b) many commonly used strings (e.g. attribute names) are interned meaning that the cache is highly effective.
Neither of these factors apply to float or complex.
There was a problem hiding this comment.
@trotterdylan Yes, I agree with you. It is not a heavy operation to caching it.
|
@trotterdylan PTAL |
trotterdylan
left a comment
trotterdylan
left a comment
There was a problem hiding this comment.
This PR looks great! Thanks for putting it together. Just a couple minor style nitpicks.
| // NewComplex returns a new Complex holding the given complex value. | ||
| func NewComplex(value complex128) *Complex { | ||
| return &Complex{Object{typ: ComplexType}, value} | ||
| return &Complex{Object: Object{typ: ComplexType}, value: value} |
There was a problem hiding this comment.
This change is no longer necessary
|
|
||
| func complexHash(f *Frame, o *Object) (*Object, *BaseException) { | ||
| v := toComplexUnsafe(o) | ||
| hashreal := hashFloat(real(v.Value())) |
There was a problem hiding this comment.
Style nit: I don't think you can reduce the number of temporaries without affecting readability too much.
|
@trotterdylan Thank you for your review! I updated with you pointed it. |
trotterdylan
left a comment
trotterdylan
left a comment
There was a problem hiding this comment.
Thanks for the changes. This looks great!
2 participants
Implement complex hash.