799490 - (harmony:strreverse) implement String.prototype.reverse

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[:Benjamin Peterson]

The ES6 String.prototype.reverse method returns a new string with all the characters reversed. It is specified in the PDF attached to the bug URL.

This is a simple method and would thus be a excellent chance for someone to dive into SpiderMonkey development.

Comment 1

[Sankha Narayan Guria [:sankha]]

I would like to work on this. I'll be needing some guidance from you though as I am new to the JavaScript Engine Component!

Comment 2

[duke]

I am interested in this as well and will start writing tests, so somebody else can start with the implementation:

Here is some help that Benjamin gave me on Twitter:

https://twitter.com/gutworth/status/255762848502910976

Comment 3

[:Benjamin Peterson]

Hi, Sankha. The main code you'll need to modify is js/src/jsstr.cpp. Imitate how other methods are written. You'll also need to add a test; probably in js/src/jit-test/tests/basic/string-reverse.js. Look at the string-contains/string-startswith.js files for inspiration.

Feel free to stop by #jsapi on Mozilla IRC [1]. (I'm "benjamin".)

Here are some general webpages that should be useful:

 - https://developer.mozilla.org/en-US/docs/Introduction
 - https://developer.mozilla.org/en-US/docs/SpiderMonkey

[1] https://wiki.mozilla.org/IRC

Comment 4

[Sankha Narayan Guria [:sankha]]

Attachment: WIP patch

I have written a method for reverse. Please give your feedback, then I can begin with the tests!

Comment 5

[:Benjamin Peterson]

Comment on attachment 669737 [details] [diff] [review]
WIP patch

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

Good start. You should comment your code with the exact steps you're performing for the spec. See str_startsWith and str_contains for examples of how to do this.

::: js/src/jsstr.cpp
@@ +564,5 @@
>  static JSBool
> +str_reverse(JSContext *cx, unsigned argc, Value *vp)
> +{
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +    

Your empty lines have trailing spaces. You should remove that.

@@ +569,5 @@
> +    RootedString str(cx, ThisToStringForStringProto(cx, args));
> +    if (!str)
> +        return false;
> +    
> +    uint32_t len = str->length();

size_t

@@ +570,5 @@
> +    if (!str)
> +        return false;
> +    
> +    uint32_t len = str->length();
> +    const jschar *s = str->getChars(cx);

You should do this as close to the use of |s| as possible. The next line allocates memory and could potentially GC. GC could cause the char pointer to move leaving |s| invalid. In fact, I suggest you write the main reversal in its own block like this:
{
    const jschar *s = str->getChars(cx);
    // do reversing
}

This ensures the unsafe pointer is visible for the least amount of time possible.

You also need to test that |s| isn't nullptr.

@@ +575,5 @@
> +    jschar *rev = cx->pod_malloc<jschar>(len + 1);
> +    if (!rev)
> +        return false;
> +    for (size_t i = 0; i < len; i++)
> +        rev[i] = s[len-i-1];

Space between "-" and operands.

@@ +579,5 @@
> +        rev[i] = s[len-i-1];
> +    rev[len] = 0;
> +    str = js_NewString(cx, rev, len);
> +    if (!str) {
> +        js_free(rev);

cx->free(rev);

@@ +3123,5 @@
>  
>      /* Java-like methods. */
>      JS_FN(js_toString_str,     js_str_toString,       0,0),
>      JS_FN(js_valueOf_str,      js_str_toString,       0,0),
> +    JS_FN("reverse",           str_reverse,           2,JSFUN_GENERIC_NATIVE),

|reverse| doesn't take an formals, so 2 should be 0.

Comment 6

[Norbert Lindenberg]

The Ecma TC 39 meeting in November 2011 decided to drop String.prototype.reverse from ES6 because of the lack of use cases.
https://mail.mozilla.org/pipermail/es-discuss/2011-November/018581.html

I recommend closing this ticket as "won't fix".

Comment 7

[:Benjamin Peterson]

Oh, indeed. The draft spec is very old (March '11). I should have been looking at this: http://wiki.ecmascript.org/doku.php?id=harmony:specification_drafts

Sorry, guys. Thanks for your work anyway.

Comment 8

[Sankha Narayan Guria [:sankha]]

(In reply to Benjamin Peterson [:benjamin] from comment #7)
> Oh, indeed. The draft spec is very old (March '11). I should have been
> looking at this:
> http://wiki.ecmascript.org/doku.php?id=harmony:specification_drafts
> 
> Sorry, guys. Thanks for your work anyway.

Ok. But are there any bugs get started with in the JS Engine? I mean I would like work with it?

Comment 9

[:Benjamin Peterson]

You could see if there's anything interesting here: http://www.joshmatthews.net/bugsahoy/?jseng=1