[webkit-reviews] review denied: [Bug 37722] Allow to construct HashTraits<WebCore::QualifiedName>::constructDeletedValue : [Attachment 53549] Patch

[bugzilla-daemon at webkit.org]

Maciej Stachowiak <mjs at apple.com> has denied anton muhin
<antonm at chromium.org>'s request for review:
Bug 37722: Allow to construct
HashTraits<WebCore::QualifiedName>::constructDeletedValue
https://bugs.webkit.org/show_bug.cgi?id=37722

Attachment 53549: Patch
https://bugs.webkit.org/attachment.cgi?id=53549&action=review

------- Additional Comments from Maciej Stachowiak <mjs at apple.com>
The ChangeLog needs to explain the purpose of this change. I can't tell if this
is fixing a bug, changing performance, or just a refactoring change.

> Index: WebCore/dom/QualifiedName.cpp
> ===================================================================
> --- WebCore/dom/QualifiedName.cpp	(revision 57720)
> +++ WebCore/dom/QualifiedName.cpp	(working copy)
> @@ -78,6 +78,7 @@ void QualifiedName::deref()
>      if (!m_impl)
>	   return;
>  #endif
> +    ASSERT(m_impl); // HashTableDeletedValue values must be never
destructed.

Hash table deleted values are generally are generally -1 cast to a pointer
rather than 0 cast to a pointer by convention.

>  
>      if (m_impl->hasOneRef())
>	   gNameCache->remove(m_impl);
> Index: WebCore/dom/QualifiedName.h
> ===================================================================
> --- WebCore/dom/QualifiedName.h	(revision 57720)
> +++ WebCore/dom/QualifiedName.h	(working copy)
> @@ -58,6 +58,8 @@ public:
>  
>      QualifiedName(const AtomicString& prefix, const AtomicString& localName,
const AtomicString& namespaceURI);
>      QualifiedName(const AtomicString& prefix, const char* localName, const
AtomicString& namespaceURI);
> +    QualifiedName(WTF::HashTableDeletedValueType) : m_impl(0) { }
> +    bool isHashTableDeletedValue() const { return !m_impl; }
>      ~QualifiedName() { deref(); }
>  #ifdef QNAME_DEFAULT_CONSTRUCTOR
>      QualifiedName() : m_impl(0) { }
> @@ -167,7 +169,7 @@ namespace WTF {
>      template<> struct HashTraits<WebCore::QualifiedName> :
GenericHashTraits<WebCore::QualifiedName> {
>	   static const bool emptyValueIsZero = false;
>	   static WebCore::QualifiedName emptyValue() { return
WebCore::QualifiedName(WebCore::nullAtom, WebCore::nullAtom,
WebCore::nullAtom); }
> -	   static void constructDeletedValue(WebCore::QualifiedName& slot) {
new (&slot) WebCore::QualifiedName(WebCore::nullAtom,
WebCore::AtomicString(HashTableDeletedValue), WebCore::nullAtom); }
> +	   static void constructDeletedValue(WebCore::QualifiedName& slot) {
new (&slot) WebCore::QualifiedName(WTF::HashTableDeletedValue); }
>	   static bool isDeletedValue(const WebCore::QualifiedName& slot) {
return slot.localName().isHashTableDeletedValue(); }
>      };
>  }

r- for not explaining the purpose of the patch. Please resubmit with an
explanation of what bug is being fixed (if any) and a test case of there is a
behavior change.