[webkit-reviews] review denied: [Bug 30658] Uninitialized memory access in svg SynchronizableProperty hashing : [Attachment 41630] Remove shouldSynchronize from hash functions

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Matt Mueller <mattm at chromium.org>'s
request for review:
Bug 30658: Uninitialized memory access in svg SynchronizableProperty hashing
https://bugs.webkit.org/show_bug.cgi?id=30658

Attachment 41630: Remove shouldSynchronize from hash functions
https://bugs.webkit.org/attachment.cgi?id=41630&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
If the whole point here is to record a set of properties and a flag for which
ones need synchronization, then I suggest using a
HashMap<SVGAnimatedPropertyBase*, bool> rather than a HashSet of a new type.
Then we could get rid of SynchronizableProperty, SynchronizablePropertyHash,
and SynchronizablePropertyHashTraits.

Another option would be to use two HashSet<SVGAnimatedPropertyBase*> -- one for
all properties, and another that contains only properties that require
synchronization. That could still be done without a custom hash.

>      bool operator==(const SynchronizableProperty& other) const
>      {
> -	   return base == other.base && shouldSynchronize ==
other.shouldSynchronize;
> +	   return base == other.base;
>      }

If we don't take my suggestion above, then I think it's poor style to have an
"==" operation that will cause two object to compare as equal if some of their
state is not identical. I suggest changing SynchronizablePropertyHash::equal
instead. I also suggest removing this overload of operator== since it is
unlikely there is code that relies on it.

>      static unsigned hash(const SynchronizableProperty& key)
>      {
> -	   return StringImpl::computeHash(reinterpret_cast<const UChar*>(&key),
sizeof(SynchronizableProperty) / sizeof(UChar));
> +	   return StringImpl::computeHash(reinterpret_cast<const
UChar*>(&key.base), sizeof(key.base) / sizeof(UChar));
>      }

If we don't take my suggestion above, then since we are hashing just a pointer,
it's far better to use PtrHash<SVGAnimatedPropertyBase*>::hash instead of
StringImpl::computeHash.