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.

Sign up for GitHub

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

Jump to bottom

math: document that Min()/Max() don't agree with builtin min()/max() #60616

Closed
crisman opened this issue Jun 6, 2023 · 6 comments
Closed

math: document that Min()/Max() don't agree with builtin min()/max() #60616

crisman opened this issue Jun 6, 2023 · 6 comments
Labels
Documentation NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.21

Comments

@crisman
Copy link
Contributor

crisman commented Jun 6, 2023

What version of Go are you using (go version)?

https://go.dev/play/?v=gotip
devel go1.21-05293d6b49 Mon Jun 5 14:01:09 2023 +0000

What did you do?

https://go.dev/play/p/Sf_G8Lt9dHy?v=gotip

	x := math.NaN()
	y := math.Inf(-1)

	fmt.Println(min(x, y))
	fmt.Println(math.Min(x, y))

Reports different output.

What did you expect to see?

I expected all output to be NaN.

What did you see instead?

min()/max() are returning NaN and math.Min()/math.Max() are returning -Inf/+Inf.

I think the output of the math versions is wrong as it should check for NaN before the infinities, but I don't have a copy of 754 right now and what min/max is under 754 is special (e.g. see minNum in IEEE 754-2008 vs. minimum & minimumNumber in IEEE 754-2019.
This was referenced Jun 6, 2023
Open
Closed
@zephyrtronium
Copy link
Contributor

This was explicitly addressed in the min/max proposal: #59488 (comment)

@crisman
Copy link
Contributor Author

crisman commented Jun 6, 2023

Ah, good point. Well given that comment and #59488 (comment) I think we should still fix math.Min/Max.

@ianlancetaylor
Copy link
Contributor

Unfortunately I don't think we can change math.Min at this point. We would need a very good reason to break compatibility, and "not quite the same as min" isn't a good enough reason.

@dr2chase
Copy link
Contributor

dr2chase commented Jun 6, 2023

I'm going to close, at minimum this would need to be a proposal and though I am sympathetic, I can imagine someone writing code that depends on the current behavior. It would be an interesting experiment to instrument math.Min/Max and see how often the problematic case occurred, if at all.

@dr2chase dr2chase closed this as completed Jun 6, 2023
@gopherbot
Copy link

Change https://go.dev/cl/501355 mentions this issue: math: document that Min/Max differ from min/max

@dmitshur dmitshur added this to the Go1.21 milestone Jun 7, 2023
@dmitshur dmitshur added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Documentation NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jun 7, 2023
@dmitshur
Copy link
Contributor

Retitling this to be about documenting this difference, which CL 501355 will resolve.

@dmitshur dmitshur reopened this Jun 11, 2023
@dmitshur dmitshur changed the title math: Min()/Max() don't agree with builtin min()/max() math: document that Min()/Max() don't agree with builtin min()/max() Jun 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Documentation NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.21
Development

No branches or pull requests
6 participants
@crisman @zephyrtronium @dmitshur @dr2chase @ianlancetaylor @gopherbot