The default bug view has changed. See this FAQ.
Bug 772733 (harmony:stringextras)

implement harmony string methods

RESOLVED FIXED in mozilla17

Status

()

Core
JavaScript Engine
--
enhancement
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: Benjamin, Assigned: Benjamin)

Tracking

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

unspecified
mozilla17
dev-doc-complete
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

5 years ago
Created attachment 641194 [details] [diff] [review]
add startsWith/endsWith
Assignee: general → bpeterson
Attachment #641194 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 2

5 years ago
Created attachment 641218 [details] [diff] [review]
String.contains()
Attachment #641194 - Attachment is obsolete: true
Attachment #641194 - Flags: review?(jwalden+bmo)
Attachment #641218 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

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

Comment 3

5 years ago
Created attachment 641219 [details] [diff] [review]
String.toArray()
Attachment #641219 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

5 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

5 years ago
Created attachment 642690 [details] [diff] [review]
add startsWith/endsWith
Attachment #641194 - Attachment is obsolete: true
Attachment #642690 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 8

5 years ago
Created attachment 642706 [details] [diff] [review]
String.contains
Attachment #641218 - Attachment is obsolete: true
Attachment #642706 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 9

5 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

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

Updated

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

Updated

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

Updated

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

Updated

5 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

5 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 15

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

5 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

5 years ago
Created attachment 648558 [details] [diff] [review]
startsWith/endsWith
Attachment #642690 - Attachment is obsolete: true
Attachment #648558 - Flags: review?(luke)
(Assignee)

Comment 18

5 years ago
Created attachment 648563 [details] [diff] [review]
String.contains

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

5 years ago
Created attachment 648565 [details] [diff] [review]
String.contains

Forgot to qref...
Attachment #648563 - Attachment is obsolete: true
Attachment #648563 - Flags: feedback?(luke)
Attachment #648565 - Flags: feedback?(luke)

Comment 20

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

5 years ago
Attachment #648565 - Flags: feedback?(luke) → feedback+
(Assignee)

Comment 21

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f89406dc763
https://hg.mozilla.org/integration/mozilla-inbound/rev/87e7abe891a9
https://hg.mozilla.org/mozilla-central/rev/6f89406dc763
https://hg.mozilla.org/mozilla-central/rev/87e7abe891a9
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17

Updated

5 years ago
Depends on: 781796

Updated

5 years ago
Depends on: 784280

Updated

5 years ago
Depends on: 786584

Updated

5 years ago
Depends on: 788483
(Assignee)

Updated

5 years ago
Depends on: 789036
(Assignee)

Updated

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

Updated

5 years ago
Depends on: 789036

Updated

5 years ago
Depends on: 789093
(Assignee)

Updated

5 years ago
No longer depends on: 789093

Updated

5 years ago
Depends on: 793781

Comment 23

5 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?

Updated

5 years ago
Keywords: dev-doc-needed → dev-doc-complete
(Assignee)

Comment 24

5 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.
Still missing from https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/String/prototype
Keywords: dev-doc-complete → dev-doc-needed

Updated

4 years ago
status-firefox17: --- → disabled
Adjusted
https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/String/prototype
so they now show up properly.
Keywords: dev-doc-needed → dev-doc-complete

Updated

4 years ago
Depends on: 821175
(Assignee)

Updated

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

Updated

4 years ago
No longer depends on: 823980

Updated

4 years ago
Depends on: 828326
(Assignee)

Updated

4 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.