[webkit-reviews] review granted: [Bug 187569] Need to handle CodeBlock::replacement() being null. : [Attachment 344845] proposed patch.

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 187569: Need to handle CodeBlock::replacement() being null.
https://bugs.webkit.org/show_bug.cgi?id=187569

Attachment 344845: proposed patch.

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




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

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

>>> Source/JavaScriptCore/bytecode/CodeBlock.cpp:2427
>>> +	 bool hasReplacement = (replacement && replacement != this);
>> 
>> No need for null check
> 
> Why?	Technically, we don't need the whole "if ((result ==
CompilationSuccessful) != (theReplacement != this))" check either because
CodeBlock::setOptimizationThresholdBasedOnCompilationResult() is only called
with result == CompilationSuccessful from
JITToDFGDeferredCompilationCallback::compilationDidComplete() right after it
installed the replacement.  The only reason that this check is here is to catch
a bug that violates the invariant that we must have a replacement if result ==
CompilationSuccessful, and to print a meaningful error message when that bug is
detected.  So, why not check for a null check as well that is also a violation
of that invariant?
> 
> Note: (replacement != this) only detects that we have a replacement if and
only if replacement is not null.

I misread this. Ignore me