Editorial: define Math functions using algorithm steps #2122
Conversation
|
Pushed a couple commits to apply @ljharb's feedback and generally:
|
|
@michaelficarra Thanks for the feedback. Hoping to get back to this later tonight. |
|
@ryanjduffy Is this ready for another review? If so, can you mark the resolved comments above as resolved? |
|
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 |
|
@michaelficarra - just saw your note. Yes, I can do that. |
|
@ryanjduffy We should remove these paragraphs at the top of 20.3.2 as part of this PR:
|
|
Updated "integer" to "integral Number" in |
There was a problem hiding this comment.
@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.
|
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! |
|
@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. |
|
Updated! |
|
Thanks for the feedback, @bakkot. Had a couple questions I shared above but I pushed a batch changes to keep things moving. |
There was a problem hiding this comment.
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_.
|
@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. |
True, although in preambles rather than algorithm steps.
Okay. |
|
Thanks for the feedback, @jmdyck. Fixed up the suggestions other than the discussion on "implementation-approximated value." |
spec.html
Outdated
| 1. Return the smallest (closest to *-∞*) integral Number value that is not less than _n_. | ||
| </emu-alg> | ||
| <emu-note> | ||
| <p>The value of `Math.ceil(x)` is the same as the value of `-Math.floor(-x)`.</p> |
There was a problem hiding this comment.
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.
spec.html
Outdated
| 1. If _n_ is *NaN*, _n_ is *+∞*, or _n_ is *-∞*, return _n_. | ||
| 1. If _n_ is neither *+0* nor *-0*, set _onlyZero_ to *false*. | ||
| 1. If _onlyZero_ is *true*, return *+0*. | ||
| 1. Return an implementation-approximated value representing the square root of the sum of squares of the elements of _numbers_. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Nice catch. I think we should fix that here, probably by constructing a second list while iterating over the first.
Fixes tc39#2119. Signed-off-by: Ryan Duffy <ryan.duffy@lge.com>
|
I'm going to rebase and land this now; for any additional feedback, please submit an issue or a PR :-) |
|
Thanks all! Good to see it land. 141 comments is definitely a new personal record for a PR :) |
ljharb
dismissed
jmdyck’s stale review
changes appear to be addressed. any further feedback can be addressed in a followup.
|
@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. |
|
@michaelficarra - not sure if 281 comments is supposed to be encouraging or intimidating ;) |
Starting a few methods to get feedback.
Fixes #2119