maxmustermann/godot
1
0
Fork 0
Code Issues Pull requests Projects Releases Wiki Activity

Correct Variant::hash_compare()

Browse source 
There was a logic error in #7815 which made
Variant.hash_compare() == Variant.hash_compare() always true.
In an attempt to short-circuit the NaN check I made an (in hindsight) obvious
error: 10 == 12 || is_nan(10) == is_nan(12)

This will be true for all inputs, except for the NaN, not-NaN case. The macro
has been updated to now generate:

(10 == 12) || (is_nan(10) && is_nan(10))

so:

(10 == 12)   || (is_nan(10)  && is_nan(12))  = false
   False  or (False and False) is False
(10 == 10)   || (is_nan(10)  && is_nan(10))  = true
   True or (False and False) is True
(Nan == 10)  || (is_nan(NaN) && is_nan(10))  = false
   False or (True and False) is False
(Nan == Nan) || (is_nan(NaN) && is_nan(NaN)) = true
   False or (True and True) is True

Which is correct for all cases.

This bug was triggered because the hash function for floating point numbers
can very easily generate collisions for the tested Vector3(). I've also added
an extra hashing step to the float hash function to make this less likely to
occur.

This fixes #8081 and probably many more random weirdness.
This commit is contained in:
Hein-Pieter van Braam 2017-04-14 11:28:51 +02:00
parent c4d3dd1a48
commit 8ff6e53833
2 changed files with 1 additions and 19 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

18
core/hashfuncs.h
View file

@ -81,24 +81,6 @@ static inline uint32_t hash_one_uint64(const uint64_t p_int) {
return (int)v;
}
static inline uint32_t hash_djb2_one_float(float p_in, uint32_t p_prev = 5381) {
union {
float f;
uint32_t i;
} u;
// Normalize +/- 0.0 and NaN values so they hash the same.
if (p_in == 0.0f)
u.f = 0.0;
else if (Math::is_nan(p_in))
u.f = Math_NAN;
else
u.f = p_in;
return ((p_prev << 5) + p_prev) + u.i;
}
// Overload for real_t size changes
static inline uint32_t hash_djb2_one_float(double p_in, uint32_t p_prev = 5381) {
union {
double d;

2
core/variant.cpp
View file

@ -2839,7 +2839,7 @@ uint32_t Variant::hash() const {
}
#define hash_compare_scalar(p_lhs, p_rhs) \
((p_lhs) == (p_rhs)) || (Math::is_nan(p_lhs) == Math::is_nan(p_rhs))
((p_lhs) == (p_rhs)) || (Math::is_nan(p_lhs) && Math::is_nan(p_rhs))
#define hash_compare_vector2(p_lhs, p_rhs) \
(hash_compare_scalar((p_lhs).x, (p_rhs).x)) && \