Skip to content
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

check argument list length up front #4

Closed
wants to merge 1 commit into from

Conversation

michaelficarra
Copy link
Member

I'm guessing this was left over from a version that took an iterator?

Also, do we really need to do this in Math.sum? It seems like if we're going to do this anywhere, we should make it apply everywhere.

@bakkot
Copy link
Collaborator

bakkot commented Jan 23, 2024

I'm guessing this was left over from a version that took an iterator?

Not a leftover, yes an intentional decision so that the iterable-taking version could work the same way. Since that's observable (unless we get rid of the ToNumber calls) I'm inclined to leave it as-is.

Also, do we really need to do this in Math.sum? It seems like if we're going to do this anywhere, we should make it apply everywhere.

We need to do this in Math.sum because at least a couple of the reasonable algorithms for handling arbitrary-precision arithmetic can overflow if fed more than 2**53 values. That's not true for e.g. Math.max.

I guess we could leave it out and just assume implementations are OK with violating the spec in that case? But it seems better to specify somehow.

@michaelficarra
Copy link
Member Author

I don't think we need to mirror the algorithms that closely. Also, if we're just calling ToNumber for consistency with Math.min/max, I don't think that's great motivation either.

@michaelficarra
Copy link
Member Author

This isn't relevant any longer because the API does take an iterator now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants