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

Editorial: quick fixes re Math functions #2174

Merged
merged 1 commit into from
Sep 14, 2020
Merged

Conversation

jmdyck
Copy link
Collaborator

@jmdyck jmdyck commented Sep 13, 2020

(Things I missed in my review of PR #2122.)


Brief history of 'max' ('min' is similar):

ES1, ES2:

  • 15.8.2.11 max(x, y)
  • Returns the larger of the two arguments.

ES3, ES5:

  • 15.8.2.11 max( [ value1 [, value2 [, ...] ] ] )
  • Given zero or more arguments, ...
  • If no arguments are given, the result is -inf.
  • The length property of the max method is 2.

Draft 23 of ES6 deleted the full-bracketing in the heading, unclear why...

ES6:

  • 20.2.2.24 Math.max ( value1, value2 , ...values )
  • Given zero or more arguments, ...
  • If no arguments are given, the result is -inf.
  • The length property of the max method is 2.

PR #266 removed the line about the length property...

ES7+:

  • 20.2.2.24 Math.max ( value1, value2 , ...values )
  • Given zero or more arguments, ...
  • If no arguments are given, the result is -inf.

spec.html Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated
</emu-alg>
<p>The *"length"* property of the `hypot` method is 2.</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm not convinced on this approach; i strongly prefer to minimize the occurrence of explicit length notes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only way to eliminate these three explicit length statements would be for each function to have a heading with exactly 2 "normal" parameters, which would be mislead (or confuse) the reader: it would suggest that:

  • a 'proper' call to the function requires at least 2 arguments, or
  • if a call supplies less than 2 args, the 'missing' args would be *undefined*.

But neither is correct for these functions.

Personally, I think it's more important to clearly convey the function's semantics. If that results in the need for an explicit length statement, that doesn't bother me.

In fact, I think it's better to have an explicit length statement in these cases, because it underlines that the length doesn't 'match' the semantics.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ljharb I entirely agree with @jmdyck's reasoning here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jmdyck's reasoning is compelling here.

If nothing else, the current rendering is very confusing because "required parameter" within the spec has a very narrow definition, which is "rendered not as a rest parameter and not between [ ]". It so happens that that narrow definition mostly aligns with the broader and more common understanding of "required" that means "needed for the function to do the useful thing". The functions for which these definitions don't align are these Math functions, and so the explicit "length" notes are definitely editorially warranted IMO.

spec.html Outdated
@@ -27931,17 +27930,16 @@ <h1>Math.max ( _value1_, _value2_, ..._values_ )</h1>
<emu-note>
<p>The comparison of values to determine the largest value is done using the Abstract Relational Comparison algorithm except that *+0* is considered to be larger than *-0*.</p>
</emu-note>
<p>The *"length"* property of the `max` method is 2.</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here.

@@ -27950,6 +27948,7 @@ <h1>Math.min ( _value1_, _value2_, ..._values_ )</h1>
<emu-note>
<p>The comparison of values to determine the largest value is done using the Abstract Relational Comparison algorithm except that *+0* is considered to be larger than *-0*.</p>
</emu-note>
<p>The *"length"* property of the `min` method is 2.</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and here.

spec.html Outdated Show resolved Hide resolved
@jmdyck
Copy link
Collaborator Author

jmdyck commented Sep 14, 2020

force-pushed to:

Copy link
Member

@michaelficarra michaelficarra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really have a preference for implicit or explicit length.

- Math.atan2: Refer to parameters in correct order.

- Math.fround: Call ToNumber on _x_.
    Insert "or" into a condition.

- Math.{hypot,max,min}: Accept zero or more arguments.
    (max and min have done so since ES3.
    hypot has done so since it was introduced in ES6.)

- Math.round: Fix typos: "_x_" -> "_n_"
@jmdyck
Copy link
Collaborator Author

jmdyck commented Sep 14, 2020

force-pushed to:

  • make the "x32 and x64 to n32 and n64" change
  • squash everything into one commit

@ljharb ljharb self-assigned this Sep 14, 2020
@ljharb ljharb merged commit d97be3e into tc39:master Sep 14, 2020
@jmdyck jmdyck deleted the editorial branch September 15, 2020 00:45
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.

5 participants