772733 - (harmony:stringextras) implement harmony string methods

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Comment 1

[:Benjamin Peterson]

Attachment: add startsWith/endsWith

Comment 2

[:Benjamin Peterson]

Attachment: String.contains()

Comment 3

[:Benjamin Peterson]

Attachment: String.toArray()

Comment 4

[Jeff Walden [:Waldo]]

Comment on attachment 641218 [details] [diff] [review]
String.contains()

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

Every new method should be prefixed with a /* ES6 20120708 draft 15.5.4.24. */ comment.  And each method should include /* Step 1. */, /* Steps 2-4. */, and so on comments to indicate what steps are being performed at each point.  This also serves the double purpose of making clear that the code has been checked for spec-correctness.  (If you need to implement stuff out of order, list the steps in the order they're performed -- just get the basic point across.)

More substantively, and maybe you were just implementing an older draft, but you don't support the [, position] argument here.  r- for that in particular.

::: js/src/jit-test/tests/basic/string-contains.js
@@ +3,5 @@
> +assertEq("abc".contains("abc"), true);
> +assertEq("abc".contains("bc"), true);
> +assertEq("abc".contains("d"), false);
> +assertEq("abc".contains("abcd"), false);
> +assertEq("abc".contains("ac"), false);

This should test that contains is generic (works on non-string and non-String values), the length of the contains function, and that ToInteger on the position argument does the right thing (and occurs after this is converted to a string), at the very least.  I would prefer if you wrote tests that went step-by-step and exercised the functionality implemented by each, for example along lines like ecma_5/Function/function-bind.js follows.

::: js/src/jsstr.cpp
@@ +1109,5 @@
> +    RootedString str(cx, ThisToStringForStringProto(cx, args));
> +    if (!str)
> +        return false;
> +
> +    JSLinearString *patstr = ArgToRootedString(cx, args, 0);

Rooted<JSLinearString*> patstr(cx, Arg...); so you're handle-compatible; you should default to using handles and roots now.

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 641194 [details] [diff] [review]
add startsWith/endsWith

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

This is missing position argument support.  And the comments on the first patch here generalize to this one, too, regarding comments, testing, and such.

Comment 6

[Jeff Walden [:Waldo]]

Comment on attachment 641219 [details] [diff] [review]
String.toArray()

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

I don't see this method in the 20120708 draft.  What gives?

::: js/src/jsstr.cpp
@@ +2701,5 @@
> +        JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_STRING_TOO_LONG_FOR_ARRAY);
> +        return false;
> +    }
> +
> +    JSObject *result = NewDenseAllocatedArray(cx, str->length(), NULL);

Rooted<JSObject*>

@@ +2706,5 @@
> +    if (!result)
> +        return false;
> +    result->setDenseArrayInitializedLength(str->length());
> +    js::types::AddTypePropertyId(cx, result, JSID_VOID, Type::StringType());
> +    args.rval().setObject(*result);

Do this at the end of the function, please.

@@ +2709,5 @@
> +    js::types::AddTypePropertyId(cx, result, JSID_VOID, Type::StringType());
> +    args.rval().setObject(*result);
> +
> +    for (unsigned i = 0; i < str->length(); i++) {
> +        JSLinearString *c = cx->runtime->staticStrings.getUnitStringForElement(cx, str, i);

Use a Rooted<JSLinearString*> outside the loop to hold each string as it's created and before it's used to init the element.

Comment 7

[:Benjamin Peterson]

Attachment: add startsWith/endsWith

Comment 8

[:Benjamin Peterson]

Attachment: String.contains

Comment 9

[:Benjamin Peterson]

(In reply to Jeff Walden [:Waldo] (gone July 18-August 26) from comment #6)
> Comment on attachment 641219 [details] [diff] [review]
> String.toArray()
> 
> Review of attachment 641219 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't see this method in the 20120708 draft.  What gives?

It's on the wiki. I suppose we should hold off on implementing it if/when it moves into the spec?

Comment 10

[Jeff Walden [:Waldo]]

Comment on attachment 642690 [details] [diff] [review]
add startsWith/endsWith

Punting, not gonna get to this.  :-(

Comment 11

[Jeff Walden [:Waldo]]

Comment on attachment 642706 [details] [diff] [review]
String.contains

Punting, not gonna get to this.  :-(

Comment 12

[David Mandelin [:dmandelin]]

Comment on attachment 642690 [details] [diff] [review]
add startsWith/endsWith

Wrong Steve Fink. :-)

Comment 13

[Steve Fink [:sfink] [:s:]]

Comment on attachment 642706 [details] [diff] [review]
String.contains

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

::: js/src/jsstr.cpp
@@ +1134,5 @@
> +        if (!ToInteger(cx, args[1], &posDouble))
> +            return false;
> +
> +        // Step 6
> +        text += JS_MIN(textlen, JS_MAX(0, ToInt32(posDouble)));

As long as that doesn't warn about mixed signed/unsigned comparisons, I'm happy.

Comment 14

[:Benjamin Peterson]

Comment on attachment 642690 [details] [diff] [review]
add startsWith/endsWith

sfink has escaped to vacation, so on to the next victim...

Comment 15

[Luke Wagner [:luke]]

Comment on attachment 642690 [details] [diff] [review]
add startsWith/endsWith

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

Good start, but a few problems:

::: js/src/jsstr.cpp
@@ +1263,5 @@
> +
> +    if (args.length() > 1) {
> +        // Step 4
> +        double posDouble;
> +        if (!ToInteger(cx, args[1], &posDouble))

It would be nice to have these steps in order.  I don't think there is a huge perf savings from trying to group them based on args.length() > 1.

Second, based on the spec, when passed startsWith("a", undefined), 'pos' should be 0, not NaN (the result of ToInteger(undefined)).  We have a nice helper for this: args.hasDefined(1).

@@ +1267,5 @@
> +        if (!ToInteger(cx, args[1], &posDouble))
> +            return false;
> +
> +        // Step 6
> +        uint32_t position = JS_MIN(textlen, JS_MAX(0, ToInt32(posDouble)));

I don't think we want ToInt32(posDouble).  IIUC, ToInteger(v) can yield -inf, inf, NaN, and integral doubles outside the int32 range.  Also, you can use the template functions Min()/Max()  (which, btw, does not repeatedly evaluate its operands; the current concoction may evaluate ToInt32(posDouble) 4 times, iiuc).

Comment 16

[:Benjamin Peterson]

(In reply to Luke Wagner [:luke] from comment #15)
> Comment on attachment 642690 [details] [diff] [review]
> add startsWith/endsWith
> 
> Review of attachment 642690 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Good start, but a few problems:
> 
> ::: js/src/jsstr.cpp
> @@ +1263,5 @@
> > +
> > +    if (args.length() > 1) {
> > +        // Step 4
> > +        double posDouble;
> > +        if (!ToInteger(cx, args[1], &posDouble))
> 
> It would be nice to have these steps in order.  I don't think there is a
> huge perf savings from trying to group them based on args.length() > 1.

I did it mainly because it's more convenient to alter text and textlen at the bottom of this condition.

Comment 17

[:Benjamin Peterson]

Attachment: startsWith/endsWith

Comment 18

[:Benjamin Peterson]

Attachment: String.contains

Also fixed String.contains. Luke, can you confirm this is sane?

Comment 19

[:Benjamin Peterson]

Attachment: String.contains

Forgot to qref...

Comment 20

[Luke Wagner [:luke]]

Comment on attachment 648558 [details] [diff] [review]
startsWith/endsWith

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

Great!

::: js/src/jsstr.cpp
@@ +1293,5 @@
> +        if (!ToInteger(cx, args[1], &posDouble))
> +            return false;
> +
> +        // Step 6
> +        uint32_t position = Min(static_cast<double>(textlen), Max(0., posDouble));

Could you write double(textlen) and 0.0?

@@ +1299,5 @@
> +        textlen -= position;
> +    }
> +
> +    // Steps 8, 9, and 10.
> +    args.rval().setBoolean(textlen >= patlen && memcmp(text, pat, patlen * sizeof(jschar)) == 0);

How about PodEqual(text, pat, patlen)?

@@ +1336,5 @@
> +        if (!ToInteger(cx, args[1], &endPosDouble))
> +            return false;
> +
> +        // Step 6
> +        textlen = Min(static_cast<double>(textlen), Max(0., endPosDouble));

ditto

@@ +1341,5 @@
> +    }
> +
> +    // Steps 8-11
> +    text += textlen - patlen;
> +    args.rval().setBoolean(textlen >= patlen && memcmp(text, pat, patlen * sizeof(jschar)) == 0);

ditto

The 'textlen - patlen' looked all scary until I saw the next line where we branch on 'textlen >= patenlen' before using 'text'.  To be less scary-looking, could you branch on 'textlen >= patlen' before doing the subtraction?  (Perhaps by putting the 'text' expression inline.)

Comment 21

[:Benjamin Peterson]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6f89406dc763
https://hg.mozilla.org/integration/mozilla-inbound/rev/87e7abe891a9

Comment 22

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6f89406dc763
https://hg.mozilla.org/mozilla-central/rev/87e7abe891a9

Comment 23

[David Bruant]

Apparently, it's been documented:
https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/String/startsWith
https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/String/endsWith
https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/String/contains

What's the status for toArray. It seems some patched had it, the latest don't. Should a separate bug be filed? this one reopened? has it been canceled?

Comment 24

[:Benjamin Peterson]

(In reply to David Bruant from comment #23)
> Apparently, it's been documented:

Great!

> What's the status for toArray. It seems some patched had it, the latest
> don't. Should a separate bug be filed? this one reopened? has it been
> canceled?

toArray isn't in the spec anymore.

Comment 25

[Gregory Szorc [:gps]]

Still missing from https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/String/prototype

Comment 26

[Tom S]

Adjusted
https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/String/prototype
so they now show up properly.