Last Comment Bug 772733 - (harmony:stringextras) implement harmony string methods
(harmony:stringextras)
: implement harmony string methods
Status: RESOLVED FIXED
[js:t]
: dev-doc-complete
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- enhancement (vote)
: mozilla17
Assigned To: :Benjamin Peterson
:
:
Mentors:
http://wiki.ecmascript.org/doku.php?i...
Depends on: 789036 793781 1102219
Blocks: es6
  Show dependency treegraph
 
Reported: 2012-07-10 21:01 PDT by :Benjamin Peterson
Modified: 2014-11-20 05:37 PST (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
disabled


Attachments
add startsWith/endsWith (4.30 KB, patch)
2012-07-11 14:12 PDT, :Benjamin Peterson
jwalden+bmo: review-
Details | Diff | Splinter Review
String.contains() (3.06 KB, patch)
2012-07-11 15:23 PDT, :Benjamin Peterson
jwalden+bmo: review-
Details | Diff | Splinter Review
String.toArray() (4.84 KB, patch)
2012-07-11 15:24 PDT, :Benjamin Peterson
jwalden+bmo: review-
Details | Diff | Splinter Review
add startsWith/endsWith (7.21 KB, patch)
2012-07-16 13:07 PDT, :Benjamin Peterson
no flags Details | Diff | Splinter Review
String.contains (4.43 KB, patch)
2012-07-16 13:33 PDT, :Benjamin Peterson
sphink: review+
Details | Diff | Splinter Review
startsWith/endsWith (7.58 KB, patch)
2012-08-02 16:42 PDT, :Benjamin Peterson
luke: review+
Details | Diff | Splinter Review
String.contains (4.70 KB, patch)
2012-08-02 16:52 PDT, :Benjamin Peterson
no flags Details | Diff | Splinter Review
String.contains (4.70 KB, patch)
2012-08-02 16:53 PDT, :Benjamin Peterson
luke: feedback+
Details | Diff | Splinter Review

Description :Benjamin Peterson 2012-07-10 21:01:04 PDT

    
Comment 1 :Benjamin Peterson 2012-07-11 14:12:01 PDT
Created attachment 641194 [details] [diff] [review]
add startsWith/endsWith
Comment 2 :Benjamin Peterson 2012-07-11 15:23:14 PDT
Created attachment 641218 [details] [diff] [review]
String.contains()
Comment 3 :Benjamin Peterson 2012-07-11 15:24:10 PDT
Created attachment 641219 [details] [diff] [review]
String.toArray()
Comment 4 Jeff Walden [:Waldo] (remove +bmo to email) 2012-07-16 11:24:31 PDT
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] (remove +bmo to email) 2012-07-16 11:24:41 PDT
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] (remove +bmo to email) 2012-07-16 11:24:50 PDT
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 2012-07-16 13:07:59 PDT
Created attachment 642690 [details] [diff] [review]
add startsWith/endsWith
Comment 8 :Benjamin Peterson 2012-07-16 13:33:03 PDT
Created attachment 642706 [details] [diff] [review]
String.contains
Comment 9 :Benjamin Peterson 2012-07-16 13:33:53 PDT
(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] (remove +bmo to email) 2012-07-18 01:26:34 PDT
Comment on attachment 642690 [details] [diff] [review]
add startsWith/endsWith

Punting, not gonna get to this.  :-(
Comment 11 Jeff Walden [:Waldo] (remove +bmo to email) 2012-07-18 01:26:40 PDT
Comment on attachment 642706 [details] [diff] [review]
String.contains

Punting, not gonna get to this.  :-(
Comment 12 David Mandelin [:dmandelin] 2012-07-25 18:47:45 PDT
Comment on attachment 642690 [details] [diff] [review]
add startsWith/endsWith

Wrong Steve Fink. :-)
Comment 13 Steve Fink [:sfink] [:s:] 2012-07-26 15:07:05 PDT
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 2012-08-02 10:41:37 PDT
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] 2012-08-02 11:01:57 PDT
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 2012-08-02 16:38:45 PDT
(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 2012-08-02 16:42:20 PDT
Created attachment 648558 [details] [diff] [review]
startsWith/endsWith
Comment 18 :Benjamin Peterson 2012-08-02 16:52:41 PDT
Created attachment 648563 [details] [diff] [review]
String.contains

Also fixed String.contains. Luke, can you confirm this is sane?
Comment 19 :Benjamin Peterson 2012-08-02 16:53:51 PDT
Created attachment 648565 [details] [diff] [review]
String.contains

Forgot to qref...
Comment 20 Luke Wagner [:luke] 2012-08-02 22:18:44 PDT
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 23 David Bruant 2012-10-12 06:38:18 PDT
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 2012-10-12 08:28:26 PDT
(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 26 AWAY Tom Schuster [:evilpie] 2012-12-11 10:20:59 PST
Adjusted
https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/String/prototype
so they now show up properly.

Note You need to log in before you can comment on or make changes to this bug.