Closed
Bug 1260509
Opened 8 years ago
Closed 8 years ago
Implement String.prototype.padStart / padEnd
Categories
(Core :: JavaScript: Standard Library, defect)
Core
JavaScript: Standard Library
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)
6.95 KB,
patch
|
Details | Diff | Splinter Review |
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 | ||
Updated•8 years ago
|
Assignee: nobody → winter2718
Assignee | ||
Updated•8 years ago
|
Component: JavaScript Engine → JavaScript: Standard Library
Updated•8 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Comment 1•8 years ago
|
||
Gonna have dinner and flag this for review after I verify that I didn't do something dumb. ^.^
Assignee | ||
Comment 2•8 years ago
|
||
Attachment #8736093 -
Attachment is obsolete: true
Attachment #8736237 -
Flags: review?(jorendorff)
Comment 3•8 years ago
|
||
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)
Assignee | ||
Comment 5•8 years ago
|
||
(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 !
Assignee | ||
Comment 6•8 years ago
|
||
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
Assignee | ||
Comment 7•8 years ago
|
||
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
Assignee | ||
Comment 8•8 years ago
|
||
Attachment #8736237 -
Attachment is obsolete: true
Comment 10•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d3ba5b5019c6
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Comment 11•8 years ago
|
||
Documentation written down here: * https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/padStart * https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/padEnd
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•