[webkit-reviews] review denied: [Bug 85120] Add fast path for radix == 10 to numberProtoFuncToString : [Attachment 139328] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Benjamin Poulain
<benjamin at webkit.org>'s request for review:
Bug 85120: Add fast path for radix == 10 to numberProtoFuncToString
https://bugs.webkit.org/show_bug.cgi?id=85120

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=139328&action=review


review- because I believe toUStringWithRadix will not work correctly for
MIN_INT. Otherwise this patch seems great!

> Source/JavaScriptCore/ChangeLog:12
> +	   This patch shortcuts the creation of a JSValue and use
NumericStrings directly.

Should say “uses” instead of “use”.

> Source/JavaScriptCore/ChangeLog:19
> +	   integers that do not fall in the two previous optimization gets 32%
faster.

Should say “optimizations” instead of “optimization”.

> Source/JavaScriptCore/ChangeLog:23
> +	   (JSC):

Please remove this bogus line from the change log.

> Source/JavaScriptCore/runtime/NumberPrototype.cpp:344
> +    LChar buf[1 + 32]; // Worst case is radix == 2, which gives us 32
positions + sign.

32 digits is better wording than 32 positions

> Source/JavaScriptCore/runtime/NumberPrototype.cpp:351
> +	   number = -number;

When you do this to MIN_INT, it just gives you MIN_INT again. I’m not sure the
code below will work in that case. Did you test that edge case to see that it
works correctly?

> Source/JavaScriptCore/runtime/NumberPrototype.cpp:455
> +static int32_t extractRadixFromArgs(ExecState* exec)

I think you want this inlined so probably best to mark it inline.

> Source/JavaScriptCore/runtime/NumberPrototype.cpp:469
> +static EncodedJSValue integerValueToString(ExecState* exec, int32_t radix,
int32_t value)

Do you want this inlined? I think it probably should be marked inline.

> Source/JavaScriptCore/runtime/NumberPrototype.cpp:483
> +	   JSString* string;
> +	   string = jsString(globalData,
globalData->numericStrings.add(value));
> +	   return JSValue::encode(string);

Should initialize the local variable named string on the same line it’s defined
on. Or, better, just do it all in one expression:

    return JSValue::encode(jsString(globalData,
globalData->numericStrings.add(value)));

> Source/JavaScriptCore/runtime/NumberPrototype.cpp:507
> +	   JSString* string = jsString(globalData,
globalData->numericStrings.add(doubleValue));
> +	   return JSValue::encode(string);

Not sure why you did this in two lines instead of one.