Closed Bug 1260509 Opened 9 years ago Closed 9 years ago

Implement String.prototype.padStart / padEnd

Categories

(Core :: JavaScript: Standard Library, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: mrrrgn, Assigned: mrrrgn)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 2 obsolete files)

This string method pads a string with a repeating sequence until the length of the string reaches some max value. The repeated 'filler' sequence may be truncated. Ex: > "blahblah".padStart(10, "foo") 'ooblahblah' > "blahblah".padStart(11, "foo") 'fooblahblah' > "blahblah".padEnd(10, "foo") 'blahblahfo' > "blahblah".padEnd(11, "foo") 'blahblahfoo' See: https://github.com/tc39/proposal-string-pad-start-end
Assignee: nobody → winter2718
Component: JavaScript Engine → JavaScript: Standard Library
Attached patch padleftright.diff (obsolete) — Splinter Review
Gonna have dinner and flag this for review after I verify that I didn't do something dumb. ^.^
Attached patch padleftright.diff (obsolete) — Splinter Review
Attachment #8736093 - Attachment is obsolete: true
Attachment #8736237 - Flags: review?(jorendorff)
Comment on attachment 8736237 [details] [diff] [review] padleftright.diff Review of attachment 8736237 [details] [diff] [review]: ----------------------------------------------------------------- How's the speed vs. V8? It seems to me like a C++ implementation should be noticeably faster, even for simple stuff like "hi".padStart(8), because every + and += here is (I think) allocating a JSString header. String_repeat uses a trick to reduce the number of concatenations (see steps 8-9). Also check that String.prototype.padStart.length is 1. I claim it should be 1 because of this in section 17: > Every built-in Function object, including constructors, has a `length` property > whose value is an integer. Unless otherwise specified, this value is equal to the > largest number of named arguments shown in the subclause headings for the function > description. Optional parameters (which are indicated with brackets: [ ]) or rest > parameters (which are shown using the form «...name») are not included in the > default argument count. Also see NOTE 2 immediately under that. ::: js/src/builtin/String.js @@ +29,5 @@ > + if (filler === "") > + return str; > + > + // Step 9. > + let fillLen = maxLength - strLen; intMaxLength, not maxLength. I guess we would need a test with fractional maxLength, or one that tests how many times .toString()/.valueOf() gets called, to catch this? ::: js/src/tests/ecma_7/String/string-pad-start-end.js @@ +25,5 @@ > + > +assertEq("42bloop", String.prototype.padEnd.call(proxy, 7, "bloopie")); > + > +if (typeof reportCompare === "function") > + reportCompare(true, true); Move this to the end, please.
Attachment #8736237 - Flags: review?(jorendorff) → review+
The functions' length absolutely is `1`. Please also note yesterday's spec changes: passing a fillStr that toStrings to the empty string causes the string to be returned unchanged. I'm working on the test262 tests right now (https://github.com/tc39/test262/pull/564)
(In reply to ljharb from comment #4) > The functions' length absolutely is `1`. Please also note yesterday's spec > changes: passing a fillStr that toStrings to the empty string causes the > string to be returned unchanged. I'm working on the test262 tests right now > (https://github.com/tc39/test262/pull/564) Great, I'll add that before landing. Thanks !
Test added. We're compliant. As for performance, v8 is also self hosted and our implementations are very similar (only so many ways to skin a cat). I'm building v8 currently, here's what I get for our implementation: for (let i = 1; i < 11; i++) { let t0 = Date.now(); mrrrgn .padStart(STEP_SIZE * i, foo); let t1 = Date.now(); console.log(); } Morgans-MacBook-Pro:_DBG.OBJ mrrrgn$ dist/bin/js ../../../../test-scripts/padspeed.js 10000 took 0.007 S 20000 took 0.004 S 30000 took 0.005 S 40000 took 0.007 S 50000 took 0.008 S 60000 took 0.008 S 70000 took 0.005 S 80000 took 0.009 S 90000 took 0.007 S 100000 took 0.009 S Morgans-MacBook-Pro:_DBG.OBJ mrrrgn$ Morgans-MacBook-Pro:_DBG.OBJ mrrrgn$ dist/bin/js ../../../../test-scripts/padspeed.js 100000 took 0.02 S 200000 took 0.022 S 300000 took 0.024 S 400000 took 0.031 S 500000 took 0.039 S 600000 took 0.041 S 700000 took 0.054 S 800000 took 0.06 S 900000 took 0.066 S 1000000 took 0.207 S
Replacing my "repeat" for loop with a call to String_repeat improved the time. I tested v8 and we're running with the same performance. Morgans-MacBook-Pro:_DBG.OBJ mrrrgn$ dist/bin/js ../../../../test-scripts/padspeed.js padStart: 100000 took 0.004 S padStart: 200000 took 0 S padStart: 300000 took 0.001 S padStart: 400000 took 0 S padStart: 500000 took 0.001 S padStart: 600000 took 0 S padStart: 700000 took 0 S padStart: 800000 took 0.001 S padStart: 900000 took 0 S padStart: 1000000 took 0 S padStart: 1100000 took 0.002 S padStart: 1200000 took 0 S padStart: 1300000 took 0 S padStart: 1400000 took 0 S padStart: 1500000 took 0 S padStart: 1600000 took 0 S padStart: 1700000 took 0 S padStart: 1800000 took 0 S padStart: 1900000 took 0 S padStart: 2000000 took 0 S
Attachment #8736237 - Attachment is obsolete: true
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: