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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Jan 20 14:43:55 PST 2019

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

Attachment 359609: Patch


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

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) &&
> +	       RELEASE_AND_RETURN(scope,
JSValue::encode(JSBigInt::remainder(exec, WTF::get<JSBigInt*>(leftNumeric),
> +
> +	   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.

More information about the webkit-reviews mailing list