1024589 - IonMonkey: Implement FromCharCode Recover Instruction

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Nicolas B. Pierron [:nbp]]

Implement RFromCharCode in js/src/jit/Recover.cpp.
See Bug 1003801 comment 0 for explanation.

Comment 1

[Rémi WENG]

Working on it.

Comment 2

[Rémi WENG]

Attachment: bug1024589.patch

Comment 3

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8444045 [details] [diff] [review]
bug1024589.patch

Review of attachment 8444045 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good.

::: js/src/jsstr.cpp
@@ +4187,5 @@
>      return true;
>  }
>  
> +bool
> +js::str_fromCharCode_one_arg(JSContext *cx, MutableHandleValue code, MutableHandleValue rval)

nit: code should be a HandleValue and not a MutableHandleValue.

This function duplicate the logic of str_fromCharCode, we want to use this function in str_fromCharCode to duplicating the logic.

Comment 4

[Rémi WENG]

Attachment: bug1024589.patch

I can not change the variable code to HandleValue (instead of MutableHandleValue) because of the line: code.setInt32(ucode)

Comment 5

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8444082 [details] [diff] [review]
bug1024589.patch

Review of attachment 8444082 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, still a few details and it would be good to go ;)

::: js/src/jsstr.cpp
@@ +4160,1 @@
>      }

style-nit: remove the curly braces, when there is only one line and there is no confusion with the condition.

@@ +4192,5 @@
> +        rval.setString(cx->staticStrings().getUnit(ucode));
> +        return true;
> +    }
> +
> +    code.setInt32(ucode);

Remove this line, it is not used anymore.

@@ +4197,5 @@
> +
> +    jschar *chars = cx->pod_malloc<jschar>(2);
> +    if (!chars)
> +        return false;
> +    chars[0] = (jschar)ucode;

nit: Prefer the C++ cast:  jschar(ucode);

Comment 6

[Rémi WENG]

Attachment: bug1024589.patch

Comment 7

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8444097 [details] [diff] [review]
bug1024589.patch

Review of attachment 8444097 [details] [diff] [review]:
-----------------------------------------------------------------

Awesome :)
I will try it on try and land it for you.

Comment 8

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8444097 [details] [diff] [review]
bug1024589.patch

Note, this patch does not have a User name nor a patch title.  I will take your Bugzilla identifier as a reference for now, and the variant of bug title for the commit message.

Comment 9

[Nicolas B. Pierron [:nbp]]

This link targets the test server of our continuous integration.

https://tbpl.mozilla.org/?tree=Try&rev=d4210b4a8702

Comment 10

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e430f92519ce

I've landed your patch with the contribution of others.  You will find the result of it on our continuous integration system (tbpl) [1].  Later a Sheriff will take the patches and merge into mozilla-central.

Congratulation :)

[1] https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=6ddc7aee37eb

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e430f92519ce