Closed Bug 772733 (harmony:stringextras) Opened 12 years ago Closed 12 years ago

implement harmony string methods

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla17
Tracking Status
firefox17 --- disabled

People

(Reporter: Benjamin, Assigned: Benjamin)

References

(Blocks 1 open bug, )

Details

(Keywords: dev-doc-complete, Whiteboard: [js:t])

Attachments

(2 files, 6 obsolete files)

      No description provided.
Attached patch add startsWith/endsWith (obsolete) — Splinter Review
Assignee: general → bpeterson
Attachment #641194 - Flags: review?(jwalden+bmo)
Attached patch String.contains() (obsolete) — Splinter Review
Attachment #641194 - Attachment is obsolete: true
Attachment #641194 - Flags: review?(jwalden+bmo)
Attachment #641218 - Flags: review?(jwalden+bmo)
Attachment #641194 - Attachment is obsolete: false
Attachment #641194 - Flags: review?(jwalden+bmo)
Attached patch String.toArray() (obsolete) — Splinter Review
Attachment #641219 - Flags: review?(jwalden+bmo)
Keywords: dev-doc-needed
Whiteboard: [js:t]
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.
Attachment #641218 - Flags: review?(jwalden+bmo) → review-
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.
Attachment #641194 - Flags: review?(jwalden+bmo) → review-
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.
Attachment #641219 - Flags: review?(jwalden+bmo) → review-
Attached patch add startsWith/endsWith (obsolete) — Splinter Review
Attachment #641194 - Attachment is obsolete: true
Attachment #642690 - Flags: review?(jwalden+bmo)
Attached patch String.contains (obsolete) — Splinter Review
Attachment #641218 - Attachment is obsolete: true
Attachment #642706 - Flags: review?(jwalden+bmo)
(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 on attachment 642690 [details] [diff] [review]
add startsWith/endsWith

Punting, not gonna get to this.  :-(
Attachment #642690 - Flags: review?(jwalden+bmo)
Comment on attachment 642706 [details] [diff] [review]
String.contains

Punting, not gonna get to this.  :-(
Attachment #642706 - Flags: review?(jwalden+bmo)
Attachment #641219 - Attachment is obsolete: true
Attachment #642690 - Flags: review?(jorendorff)
Attachment #642706 - Flags: review?(jorendorff)
Attachment #642690 - Flags: review?(jorendorff) → review?(sfink)
Attachment #642706 - Flags: review?(jorendorff) → review?(sfink)
Comment on attachment 642690 [details] [diff] [review]
add startsWith/endsWith

Wrong Steve Fink. :-)
Attachment #642690 - Flags: review?(sfink) → review?(sphink)
Attachment #642706 - Flags: review?(sfink) → review?(sphink)
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.
Attachment #642706 - Flags: review?(sphink) → review+
Comment on attachment 642690 [details] [diff] [review]
add startsWith/endsWith

sfink has escaped to vacation, so on to the next victim...
Attachment #642690 - Flags: review?(sphink) → review?(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).
Attachment #642690 - Flags: review?(luke)
(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.
Attachment #642690 - Attachment is obsolete: true
Attachment #648558 - Flags: review?(luke)
Attached patch String.contains (obsolete) — Splinter Review
Also fixed String.contains. Luke, can you confirm this is sane?
Attachment #642706 - Attachment is obsolete: true
Attachment #648563 - Flags: feedback?(luke)
Attached patch String.containsSplinter Review
Forgot to qref...
Attachment #648563 - Attachment is obsolete: true
Attachment #648563 - Flags: feedback?(luke)
Attachment #648565 - Flags: feedback?(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.)
Attachment #648558 - Flags: review?(luke) → review+
Attachment #648565 - Flags: feedback?(luke) → feedback+
https://hg.mozilla.org/mozilla-central/rev/6f89406dc763
https://hg.mozilla.org/mozilla-central/rev/87e7abe891a9
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Depends on: 781796
Depends on: 784280
Depends on: 786584
Depends on: 788483
Depends on: 789036
No longer depends on: 781796, 784280, 786584, 788483, 789036
Depends on: 789036
Depends on: 789093
No longer depends on: 789093
Depends on: 793781
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?
(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.
Depends on: 821175
No longer depends on: 821175
Depends on: 823980
No longer depends on: 823980
Depends on: 828326
No longer depends on: 828326
Depends on: 1102219
You need to log in before you can comment on or make changes to this bug.