-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix ES6 polyfills to use ToInteger and ToLength #3216
Conversation
@@ -58,6 +58,7 @@ testSuite({ | |||
assertFails(RangeError, () => String.fromCodePoint(noCheck({}))); | |||
assertFails(RangeError, () => String.fromCodePoint(NaN)); | |||
assertFails(RangeError, () => String.fromCodePoint(noCheck(/./))); | |||
assertFails(RangeError, () => String.fromCodePoint(noCheck(undefined))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although it is already working, I added a test just in case.
Created internal issue b/123659800 |
opt_end = Number(opt_end); | ||
if (opt_end < 0) opt_end = Math.max(0, length + opt_end); | ||
for (var i = Number(opt_start || 0); i < opt_end; i++) { | ||
var end = opt_end === undefined ? length : $jscomp.toInteger(opt_end); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a lot of repetitive code here and in other files. What do you think about any of the following:
$jscomp.toInteger
takes a second optional argument for what to return on undefined (may not be quite as smooth as just checking isNaN tho since we still want NaN to map to zero even if undefined maps to length - so not sure if it ends up being worth it).- add a
$jscomp.toPosition(index, length)
that handles negative indices and clamping all in one place (but would be nice to handle undefined here as well, and sometimes that's 0 and sometimes it's length - but at least we could just call$jscomp.toPosition(opt_start !== undefined ? opt_start : length, length)
)
Closing this since there have been no comments for over a year. |
from #2902 (comment)
This change improves compatibility with ECMAScript spec.
Use
ToInteger
to support coercing, null, undefined, float, NaN and larger-than-Int32.Also use
ToLength
to handle negative values and larger-than-2^53.