[webkit-reviews] review denied: [Bug 122627] DFG: Add JIT support for LogicalNot(String/StringIdent) : [Attachment 213950] Patch

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has denied Nadav Rotem <nrotem at apple.com>'s
request for review:
Bug 122627: DFG: Add JIT support for  LogicalNot(String/StringIdent)
https://bugs.webkit.org/show_bug.cgi?id=122627

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

------- Additional Comments from Filip Pizlo <fpizlo at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=213950&action=review


Almost good, just needs a few fixes.

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:573
>		   break;
> +	       case StringUse:
>	       case ObjectOrOtherUse:

I think that the StringUse case should be in the case block above, since the
only reason why it would exit is for the type check.  AbstractInterpreter
already will setCanExit(true) if it determines that the type check implied by
the use kind is necessary.

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:296
> +	       else if (node->child1()->shouldSpeculateString() ||
node->child1()->shouldSpeculateStringIdent())
> +		   fixEdge<StringUse>(node->child1());

shouldSpeculateString() should subsume shouldSpeculateStringIdent(), so you
don't need the second part.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4170
> +void SpeculativeJIT::compileStringIdentZeroLength(Node* node)

See below about method name.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:1663
> +    case StringUse:
> +	   return compileStringIdentZeroLength(node);
> +

I wouldn't call this method compileStringIdentZeroLength.  It doesn't have
anything to do with StringIdent - you're not requiring the operand to be an
identifier, you're only requiring it to be a string.  So, a name like
compileStringZeroLength would be fine.