JuliaSymbolics / SymbolicUtils.jl

Symbolic expressions, rewriting and simplification
https://docs.sciml.ai/SymbolicUtils/stable/
Other
523 stars 99 forks source link

Not a good practice to put seed as the 1st argument in hash function #592

Open bowenszhu opened 2 months ago

bowenszhu commented 2 months ago

https://github.com/JuliaSymbolics/SymbolicUtils.jl/blob/e9a96bd835d742cc7992722ede008b5cfbad7eb9/src/types.jl#L254-L269

Note line 267 in the above code. hashoffset of type UInt, serving as a seed, is the first argument of the hash function. Although it still works, it somehow violates the original design principle of 2-argument hash functions.

For hash(x::Int64, h::UInt), Julia Base focuses on the first argument and merely does a scalar multiplication and subtraction to the second argument, which is probably not what we want. https://github.com/JuliaLang/julia/blob/57b9b591e28181aee8ff985d0ace661b408577d7/base/hashing.jl#L88 https://github.com/JuliaLang/julia/blob/57b9b591e28181aee8ff985d0ace661b408577d7/base/hashing.jl#L79 https://github.com/JuliaLang/julia/blob/57b9b591e28181aee8ff985d0ace661b408577d7/base/hashing.jl#L44

nsajko commented 1 month ago

hash(::UInt, ::UInt) just combines the arguments in a noncommutative way, with a multiplication and subtraction as you say. So it's quite cheap, but you could save a cycle or two by using for mixing hashoffset with the computed hash instead of calling hash(::UInt, ::UInt).