Closed Bug 1024589 Opened 8 years ago Closed 8 years ago

IonMonkey: Implement FromCharCode Recover Instruction


(Core :: JavaScript Engine: JIT, defect)

Not set





(Reporter: nbp, Assigned: contact, Mentored)


(Blocks 1 open bug)


(Whiteboard: [good first bug][lang=c++])


(1 file, 2 obsolete files)

Implement RFromCharCode in js/src/jit/Recover.cpp.
See Bug 1003801 comment 0 for explanation.
Mentor: nicolas.b.pierron
Whiteboard: [good first bug][mentor=nbp][lang=c++] → [good first bug][lang=c++]
Working on it.
Assignee: nobody → wengremi
Attached patch bug1024589.patch (obsolete) — Splinter Review
Attachment #8444045 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8444045 [details] [diff] [review]

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.
Attachment #8444045 - Flags: review?(nicolas.b.pierron) → feedback+
Attached patch bug1024589.patch (obsolete) — Splinter Review
I can not change the variable code to HandleValue (instead of MutableHandleValue) because of the line: code.setInt32(ucode)
Attachment #8444082 - Flags: review?(nicolas.b.pierron)
Attachment #8444045 - Attachment is obsolete: true
Comment on attachment 8444082 [details] [diff] [review]

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);
Attachment #8444082 - Flags: review?(nicolas.b.pierron) → feedback+
Attachment #8444082 - Attachment is obsolete: true
Attached patch bug1024589.patchSplinter Review
Attachment #8444097 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8444097 [details] [diff] [review]

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

Awesome :)
I will try it on try and land it for you.
Attachment #8444097 - Flags: review?(nicolas.b.pierron) → review+
Comment on attachment 8444097 [details] [diff] [review]

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.
This link targets the test server of our continuous integration.

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 :)

Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.