Editorial: define Math functions using algorithm steps

[ryanjduffy]

Starting a few methods to get feedback.

Fixes #2119

[ryanjduffy]

Pushed a couple commits to apply @ljharb's feedback and generally:

• Separate conditions with commas (If _condition_, or _condition_, or _condition_, then ...)
• Use is *NaN* over Number.isNaN and is finite over Number.isFinite

[ryanjduffy]

@michaelficarra Thanks for the feedback. Hoping to get back to this later tonight.

[michaelficarra]

@ryanjduffy Is this ready for another review? If so, can you mark the resolved comments above as resolved?

[ryanjduffy]

Finished converting the methods but haven't reviewed the code yet. Reviews are still welcome but I won't move this out of draft until I've had a chance to review in detail.

cc: @ljharb, @michaelficarra

[ryanjduffy]

@michaelficarra - just saw your note. Yes, I can do that.

[michaelficarra]

@ryanjduffy We should remove these paragraphs at the top of 20.3.2 as part of this PR:

[ryanjduffy]

Updated "integer" to "integral Number" in floor() and ceil() and removed those two paragraphs.

[michaelficarra]

@ryanjduffy This is looking great! Left some fairly minor comments, but otherwise LGTM. We should probably add a sentence at the end of each summary like "The Math.whatever function performs the following steps when called:", which is typical of built-in functions defined by algorithm steps as these now are.

@tc39/ecma262-editors I don't think we have to be super strict about our use of numeric literals in this PR since we can just take care of them in #2007.

[ryanjduffy]

I think I've addressed all the feedback so I've moved this out of draft. Thanks for the help getting up to speed with the conventions, @michaelficarra and @ljharb!

[ljharb]

@ryanjduffy none of the suggestions are blockers; feel free to make them yourself :-) i self-assigned since i plan to merge this once enough editors have stamped.

[ryanjduffy]

Updated!

[bakkot]

Looks pretty good. I had a few small comments.

[ryanjduffy]

Thanks for the feedback, @bakkot. Had a couple questions I shared above but I pushed a batch changes to keep things moving.

[jmdyck]

The phrase
an implementation-approximated value representing the result of the <foo> of _n_
is rather long. I don't think "the result of" is necessary, so I suggest dropping it:
an implementation-approximated value representing the <foo> of _n_

Interestingly, there's nothing about "implementation-approximated value" that says it's a Number. Nor is there an over-arching requirement that allows us to infer that the value must be a Number. (The first para of this clause used to say "The value returned by each function is a Number", but this PR removed it.) So maybe reinstate that sentence, or insert "Number":
an implementation-approximated Number value representing the <foo> of _n_

Moreover, I think the "representing" is incorrect. The resulting Number value doesn't represent the <foo> of _n_; rather it represents a mathematical value that approximates the <foo> of _n_. So you could maybe say:
a Number value that implementation-approximates the <foo> of _n_
but then you don't get auto-linking to "implementation-approximated".

So you could coin a phrase, e.g.
a Number value that approximates _x_
or
a Number-approximation for _x_
and then define it in 6.1.6.1 "The Number Type", right after defining the Number value for _x_, and mention there that it's implementation-approximated.

Note that an implementation-approximated facility is supposed to recommend "an ideal behaviour". So you could say that for a Number value that approximates _x_, the ideal behaviour is the Number value for _x_.

[bakkot]

@jmdyck I agree that the "an implementation-approximated value representing the result of of n" phrasing is awkward and a little imprecise, but its use for math predates this PR; I think we shouldn't worry about changing it here, and instead explore alternative phrasings in a different issue.

[jmdyck]

its use for math predates this PR

True, although in preambles rather than algorithm steps.

I think we shouldn't worry about changing it here, and instead explore alternative phrasings in a different issue.

Okay.

[ryanjduffy]

Thanks for the feedback, @jmdyck. Fixed up the suggestions other than the discussion on "implementation-approximated value."

[bakkot]

LGTM. Thanks for sticking with it!

[ljharb]

Fully re-reviewed this; my comments are nonblockers and should be addressed in future PRs.

[ljharb]

i wonder if it'd be simpler to just have this algorithm return - %Math.floor(-x)% rather than repeating the steps and having the note.

[ljharb]

Given that the elements of numbers aren't actually all numbers - they might be objects that are ToNumbered - it seems like this actually mandates two observable ToNumber calls for any object value.

Obviously this is not the intent, and it's likely that all implementations only call ToNumber once per item, but I think we should lock this down in a future PR.

[bakkot]

Nice catch. I think we should fix that here, probably by constructing a second list while iterating over the first.

[ljharb]

I'm going to rebase and land this now; for any additional feedback, please submit an issue or a PR :-)

[ryanjduffy]

Thanks all! Good to see it land. 141 comments is definitely a new personal record for a PR :)

changes appear to be addressed. any further feedback can be addressed in a followup.

[michaelficarra]

@ryanjduffy Yeah we're pretty thorough here. #2045 is currently sitting at 281 comments. If you want to take up any more open issues, I'd be happy to help out.

[ryanjduffy]

@michaelficarra - not sure if 281 comments is supposed to be encouraging or intimidating ;)