Closed Bug 1024589 Opened 10 years ago Closed 10 years ago

IonMonkey: Implement FromCharCode Recover Instruction

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: nbp, Assigned: contact, Mentored)

References

(Blocks 1 open bug)

Details

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

Attachments

(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]
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.
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]
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);
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]
bug1024589.patch

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

https://tbpl.mozilla.org/?tree=Try&rev=d4210b4a8702
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
https://hg.mozilla.org/mozilla-central/rev/e430f92519ce
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.