[webkit-reviews] review denied: [Bug 172115] [DFG] Constant Folding Phase should convert MakeRope("", String) => Identity(String) : [Attachment 310136] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has denied Yusuke Suzuki
<utatane.tea at gmail.com>'s request for review:
Bug 172115: [DFG] Constant Folding Phase should convert MakeRope("", String) =>
Identity(String)
https://bugs.webkit.org/show_bug.cgi?id=172115

Attachment 310136: Patch

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




--- Comment #5 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 310136
  --> https://bugs.webkit.org/attachment.cgi?id=310136
Patch

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

Mostly LGTM, just a few comments

> Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:643
> +	       case MakeRope: {

I believe you need this code inside AbstractInterpreter as well to indicate
we've found a constant. Perhaps all of it can just be moved there.

> Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:653
> +		       const auto* string =
asString(childConstant)->tryGetValueImpl();

Nit: Can we give this a type, since it's hard to see what's going on here
without reading tryGetValueImpl()'s type?

> Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:661
> +		       // Don't allow the MakeRope to have zero children.
> +		       if (!i && !node->child2())
> +			   break;

Why not just convert to lazy constant for "" here?