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

all: support map[f32]string and map[f64]string (float map keys) too #8556

Merged

Conversation

spytheman
Copy link
Member

With this PR:

m64 := { 3.14: 'pi' }
println(m64)

works.

@spytheman spytheman marked this pull request as draft February 4, 2021 19:18
@shadowninja55
Copy link
Member

shadowninja55 commented Feb 4, 2021

perhaps change the message of line 83 of parse_type.v?
p.error_with_pos('maps only support string, integer, rune or voidptr keys for now (not $s)',

@@ -2471,7 +2471,7 @@ fn (mut g Gen) map_fn_ptrs(key_typ table.TypeSymbol) (string, string, string, st
key_eq_fn = '&map_eq_int_2'
clone_fn = '&map_clone_int_2'
}
.int, .u32, .rune {
.int, .u32, .rune, .f32 {
hash_fn = '&map_hash_int_4'
key_eq_fn = '&map_eq_int_4'
Copy link
Contributor

@ntrel ntrel Feb 4, 2021

Choose a reason for hiding this comment

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

Floats were deliberately left out (for the non-string keys work) because they can have different bit patterns which still compare as equal with ==. So it should use a different equality function. The hash function probably needs to be specific to floats too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Because of the +-0 and the nans?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll add custom hash functions, I have not thought about them, thanks.

Copy link
Member

@ka-weihe ka-weihe Feb 4, 2021

Choose a reason for hiding this comment

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

I think this should work fine? However, it is a bit weird to use floats as keys in a map. Not sure when you would ever want to do that.

Copy link
Member Author

@spytheman spytheman Feb 4, 2021

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@ka-weihe it was weird to me too (I've never used floats for keys in a hash map before), but it was asked for by @shadowninja55 in the discord channel, it was easy to do, and is consistent with the rest of the language, so I think it will be a good addition.

Copy link
Member

Choose a reason for hiding this comment

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

yeah honestly i probably wouldn't use it much either, but it's always good to support everything we can.

Copy link
Contributor

Choose a reason for hiding this comment

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

If Kaspar thinks it's fine, I expect it is.

Copy link
Member

@ka-weihe ka-weihe Feb 4, 2021

Choose a reason for hiding this comment

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

Well, it is probably not going to behave like some people would expect it to... Only floats with the exact same bits will match.

@ntrel ntrel requested a review from ka-weihe February 4, 2021 19:33
@medvednikov
Copy link
Member

Honestly, using floats as map keys just sounds wrong.

Floats are notoriously hard to compare, and what's the use case? I'd be in favor of not allowing float keys.

@larpon
Copy link
Contributor

larpon commented Feb 4, 2021

I use them for caching common pre-calculated values on a timeline in my current game. Float keys have their usage in the real world - although it's surely less common as mentioned. They can be good for caching certain value ranges or lookups where you know how to cut the decimals cutting overhead for string conversions.

@medvednikov medvednikov marked this pull request as ready for review February 4, 2021 22:55
@medvednikov medvednikov merged commit 119dfc0 into vlang:master Feb 4, 2021
@medvednikov
Copy link
Member

ok, sounds useful

@dumblob
Copy link
Contributor

dumblob commented Feb 6, 2021

Yes, having float keys in maps is useful. Thanks for supporting this!

But unfortunately currently float comparisons are undefined in V and from there follows, that withdrawal of values from such a map is also undefined.

It's maybe time to finally tackle this...

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.

None yet

7 participants