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

Issue with NaN not comparable so piling up in Set[float64] #13

Closed
ldemailly opened this issue Mar 24, 2023 · 3 comments · Fixed by #68
Closed

Issue with NaN not comparable so piling up in Set[float64] #13

ldemailly opened this issue Mar 24, 2023 · 3 comments · Fixed by #68
Labels
documentation Improvements or additions to documentation enhancement New feature or request

Comments

@ldemailly
Copy link
Member

ldemailly commented Mar 24, 2023

https://go.dev/play/p/2L3NR09zIJu

package main

import (
	"fmt"
	"math"

	"fortio.org/sets"
)

func test(v float64) {
	set := sets.New(v, v, v, v)
	set.Add(v, v, v)
	fmt.Printf("Using %f : %d elems %v and has: %v\n", v, len(set), set, set.Has(v))
}

func main() {
	test(math.Pi)
	test(math.NaN())
}

outputs

# As expected
Using 3.141593 : 1 elems 3.141592653589793 and has: true
# Not great
Using NaN : 7 elems NaN,NaN,NaN,NaN,NaN,NaN,NaN and has: false
@ldemailly ldemailly added the wontfix This will not be worked on label Mar 24, 2023
@ldemailly
Copy link
Member Author

Just leaving as a doc issue because otherwise would need to pepper the API and implementation with float/NaN checks

@dolmen
Copy link
Contributor

dolmen commented May 29, 2024

I think that NaN should be forbidden in New, Add, Remove, Has: just panic. Because it is better to fail early.

The cmp package has an isNaN function.

@ldemailly
Copy link
Member Author

ldemailly commented May 30, 2024

x != x is indeed a clever IsNaN() that was mentioned on gophers slack when this came up

panic for NaN is a reasonable suggestion, I wonder if there is a noticeable perf effect of adding that check, probably not when inlined

@ldemailly ldemailly added documentation Improvements or additions to documentation enhancement New feature or request and removed wontfix This will not be worked on labels May 30, 2024
ldemailly added a commit that referenced this issue Jun 4, 2024
@ldemailly ldemailly mentioned this issue Jun 4, 2024
ldemailly added a commit that referenced this issue Jun 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants