[webkit-reviews] review denied: [Bug 186174] [BigInt] Add ValueMod into DFG : [Attachment 359609] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has denied Caio Lima <ticaiolima at gmail.com>'s
request for review:
Bug 186174: [BigInt] Add ValueMod into DFG
https://bugs.webkit.org/show_bug.cgi?id=186174

Attachment 359609: Patch

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




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

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

Mostly LGTM, but you have a bug in doesGC()

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:920
> +    case ValueMod:

Would be nice to do some constant folding. It makes sense to abstract away the
constant folding done in ArithDiv. Just because we have UntypedUse does not
mean we can't constant fold.

> Source/JavaScriptCore/dfg/DFGDoesGC.cpp:108
> +    case ValueMod:

This should return true when BigIntUse. Also, same for all of the above
operations since they can allocate a BigInt.

> Source/JavaScriptCore/dfg/DFGOperations.cpp:376
> +    if (WTF::holds_alternative<JSBigInt*>(leftNumeric) ||
WTF::holds_alternative<JSBigInt*>(rightNumeric)) {
> +	   if (WTF::holds_alternative<JSBigInt*>(leftNumeric) &&
WTF::holds_alternative<JSBigInt*>(rightNumeric))
> +	       RELEASE_AND_RETURN(scope,
JSValue::encode(JSBigInt::remainder(exec, WTF::get<JSBigInt*>(leftNumeric),
WTF::get<JSBigInt*>(rightNumeric))));
> +
> +	   return throwVMTypeError(exec, scope, "Invalid mix of BigInt and
other type in remainder operation.");
> +    }

Nit: There is a lot of code w.r.t BigInts that use this same branch pattern.
It'd be nice to just generalize it in some function that accepts lambdas.