[webkit-reviews] review granted: [Bug 206430] Deleting a property should not turn structures into uncacheable dictionaries : [Attachment 389597] Patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <ysuzuki at apple.com> has granted Justin Michaud
<justin_michaud at apple.com>'s request for review:
Bug 206430: Deleting a property should not turn structures into uncacheable
dictionaries
https://bugs.webkit.org/show_bug.cgi?id=206430

Attachment 389597: Patch

https://bugs.webkit.org/attachment.cgi?id=389597&action=review




--- Comment #11 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 389597
  --> https://bugs.webkit.org/attachment.cgi?id=389597
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=389597&action=review

r=me

> Source/JavaScriptCore/runtime/PropertyMapHashTable.h:360
> +#ifndef NDEBUG

Use #if ASSERT_ENABLED

> Source/JavaScriptCore/runtime/PropertyMapHashTable.h:497
> +#ifndef NDEBUG

Ditto.

> Source/JavaScriptCore/runtime/Structure.cpp:564
> +    if (Structure* existingTransition =
structure->m_transitionTable.get(propertyName.uid(), attributes, false)) {

Instead of passing `false`, let's pass,

constexpr bool isAddition = false;
if (Structure* existingTransition =
structure->m_transitionTable.get(propertyName.uid(), attributes, isAddition)) {

> Source/JavaScriptCore/runtime/Structure.cpp:581
> +    for (auto* s = structure; s; s = s->previousID())
> +	   ++transitionCount;

Let's stop this iteration when transitionCount exceeds s_maxTransitionLength

> Source/JavaScriptCore/runtime/StructureTransitionTable.h:185
> +    bool contains(UniquedStringImpl*, unsigned attributes, bool isAddition =
true) const;
> +    Structure* get(UniquedStringImpl*, unsigned attributes, bool isAddition
= true) const;

I think specifying `isAddition` boolean in the caller side is easier to read
instead of using default parameter.