[webkit-reviews] review denied: [Bug 203867] JSGlobalObject::fireWatchpointAndMakeAllArrayStructuresSlowPut() should fire its watchpoint as the last step. : [Attachment 382933] proposed patch.

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has denied Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 203867: JSGlobalObject::fireWatchpointAndMakeAllArrayStructuresSlowPut()
should fire its watchpoint as the last step.
https://bugs.webkit.org/show_bug.cgi?id=203867

Attachment 382933: proposed patch.

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




--- Comment #10 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 382933
  --> https://bugs.webkit.org/attachment.cgi?id=382933
proposed patch.

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

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:8004
> +    if (!globalObject->hasStartedTransitioningToHavingABadTime() &&
!isHavingABadTime && !hasAnyArrayStorage(node->indexingType())) {

this is super localized to this one place (there is probably other code you're
not catching). It's also not really needed. I'm not a fan of adding more code
to appease an assertion like this with a huge comment. It seems better to just
remove the assertion if that's valid. (Also note: the FTL has no such
assertion.) It's not worth bending over backwards for an assertion we know is
ultimately wrong.