Editorial: take ToString and ToLength out of StringPad and make it infallible

[dewren99]

Changed argument types and removed ToString and ToLength calls from StringPad as per #2634

For maxLength, I went with a non-negative integer instead of a Number.
I'm not sure if the new steps for String.prototype.padStart and String.prototype.padEnd are needed or not.

Please let me know if any further changes are needed, thanks!

[michaelficarra]

Looks good otherwise. You just need to apply those suggestions where appropriate.

[dewren99]

@michaelficarra I've applied your suggestions and thank you for the detailed explanations, the changes make more sense to me now!

Also, the original issue suggested that it is editorial, but wouldn't these changes count as normative instead?

[michaelficarra]

@dewren99 Normative changes are those that would change the behaviour of some program. This change shouldn't affect the behaviour of any program. All effects occur in the same order and under the same conditions.

[michaelficarra]

@dewren99 We discussed this in the editor call today. We would like to split out the (mostly) shared implementations of padStart and padEnd into a new AO. It would basically have the same signature as the old StringPad AO but its implementation would be what you currently have in padStart/padEnd. Then padStart/padEnd get reduced to just a call to this new AO, their only difference being passing ~start~/~end~ respectively. Does that make sense? Let me know if you have any questions. Once that's done, this should be good to go.

[dewren99]

@michaelficarra sounds good! I think I understand the overall idea, but it might take a couple of tries for me to get it right.
I've split out the shared implementations of padStart and padEnd into a new AO, if you could check it that would be great!
Also, what should be the name of the new AO? As a placeholder, it is temporarily named StringPadStartEnd right now.

[michaelficarra]

@dewren99 That looks good! Names are hard. I'd probably do something like StringPaddingBuiltinsImpl. Let's use that for now, and the other editors might come up with something better during their review.

[michaelficarra]

@bakkot @syg This should be as we discussed at the last editor call now.

[syg]

lgtm!

[bakkot]

This should just be a String; the undefined case should be hoisted into StringPaddingBuiltinsImpl. Otherwise LGTM; I'll push up a commit with that change so it doesn't need to block.