Bug 772733 (harmony:stringextras)

implement harmony string methods

RESOLVED FIXED in mozilla17

Status

()

enhancement
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: Benjamin, Assigned: Benjamin)

Tracking

(Blocks 1 bug, {dev-doc-complete})

unspecified
mozilla17
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox17 disabled)

Details

(Whiteboard: [js:t], URL)

Attachments

(2 attachments, 6 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

7 years ago
Posted patch add startsWith/endsWith (obsolete) — Splinter Review
Assignee: general → bpeterson
Attachment #641194 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 2

7 years ago
Posted patch String.contains() (obsolete) — Splinter Review
Attachment #641194 - Attachment is obsolete: true
Attachment #641194 - Flags: review?(jwalden+bmo)
Attachment #641218 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

7 years ago
Attachment #641194 - Attachment is obsolete: false
Attachment #641194 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 3

7 years ago
Posted patch String.toArray() (obsolete) — Splinter Review
Attachment #641219 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

7 years ago
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-
(Assignee)

Comment 7

7 years ago
Posted patch add startsWith/endsWith (obsolete) — Splinter Review
Attachment #641194 - Attachment is obsolete: true
Attachment #642690 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 8

7 years ago
Posted patch String.contains (obsolete) — Splinter Review
Attachment #641218 - Attachment is obsolete: true
Attachment #642706 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 9

7 years ago
(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)
(Assignee)

Updated

7 years ago
Attachment #641219 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Attachment #642690 - Flags: review?(jorendorff)
(Assignee)

Updated

7 years ago
Attachment #642706 - Flags: review?(jorendorff)
(Assignee)

Updated

7 years ago
Attachment #642690 - Flags: review?(jorendorff) → review?(sfink)
(Assignee)

Updated

7 years ago
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+
(Assignee)

Comment 14

7 years ago
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)
(Assignee)

Comment 16

7 years ago
(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.
(Assignee)

Comment 17

7 years ago
Attachment #642690 - Attachment is obsolete: true
Attachment #648558 - Flags: review?(luke)
(Assignee)

Comment 18

7 years ago
Posted 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)
(Assignee)

Comment 19

7 years ago
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+

Updated

7 years ago
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
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17

Updated

7 years ago
Depends on: 781796

Updated

7 years ago
Depends on: 784280

Updated

7 years ago
Depends on: 786584

Updated

7 years ago
Depends on: 788483
(Assignee)

Updated

7 years ago
Depends on: 789036
(Assignee)

Updated

7 years ago
No longer depends on: 781796, 784280, 786584, 788483, 789036
(Assignee)

Updated

7 years ago
Depends on: 789036

Updated

7 years ago
Depends on: 789093
(Assignee)

Updated

7 years ago
No longer depends on: 789093
Depends on: 793781

Comment 23

7 years ago
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?
(Assignee)

Comment 24

7 years ago
(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.

Updated

6 years ago
Depends on: 821175
(Assignee)

Updated

6 years ago
No longer depends on: 821175
Depends on: 823980
(Assignee)

Updated

6 years ago
No longer depends on: 823980
Depends on: 828326
(Assignee)

Updated

6 years ago
No longer depends on: 828326
Depends on: 1102219
You need to log in before you can comment on or make changes to this bug.