Closed
Bug 1260509
Opened 9 years ago
Closed 9 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•9 years ago
|
Assignee: nobody → winter2718
Assignee | ||
Updated•9 years ago
|
Component: JavaScript Engine → JavaScript: Standard Library
Updated•9 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Comment 1•9 years ago
|
||
Gonna have dinner and flag this for review after I verify that I didn't do something dumb. ^.^
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8736093 -
Attachment is obsolete: true
Attachment #8736237 -
Flags: review?(jorendorff)
Comment 3•9 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•9 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•9 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•9 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•9 years ago
|
||
Attachment #8736237 -
Attachment is obsolete: true
Comment 10•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Comment 11•9 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
•