Repository navigation
Add specializations for all functions from Generic - #322
Conversation
bc0c166 to
f5b9f57
Compare
Shimuuar
changed the title
|
Now all differences between Generic and rest of immutable vectors are accounted for. There're still mutable vectors. But that's much bigger fish to fry. |
lehins
left a comment
lehins
left a comment
There was a problem hiding this comment.
I don't feel like this is 100% correct wording for cmpBy functions:
Check if two vectors are equal using supplied comparison function
Since it's the ordering it will produce, not just equality.
But I can't think of a way to rephrase it in order to make it better. And it is definitely better than it was before, so 👍 from me.
Ah! That's case of "I copy-paste, therefore I don't think". Equality is remnant of |
| {-# INLINE eqBy #-} | ||
| eqBy = G.eqBy | ||
|
|
||
| -- | /O(n)/ Check if two vectors are equal using supplied comparison function |
There was a problem hiding this comment.
It's important to mention here and above what happens for vectors of different length.
There was a problem hiding this comment.
I finally updated haddocks for cmpBy
| -- vector elements. Comparison works same as for lists. | ||
| -- | ||
| -- > cmpBy compare == compare | ||
| cmpBy :: (a -> a -> Ordering) -> Vector a -> Vector a -> Ordering |
There was a problem hiding this comment.
Shouldn't it be cmpBy :: (a -> b -> Ordering) -> Vector a -> Vector b -> Ordering?
There was a problem hiding this comment.
Good catch. That is the type of G.cmpBy, so it seems like it should apply to specialized versions as well.
There was a problem hiding this comment.
I specialized these to a -> a -> Ordering because almost always vectors of the same type are compared and if someone needs most generic case there's Data.Vector.Generic.cmpBy
There was a problem hiding this comment.
I do not see a good reason to restrict specialized versions in such way. This is both less powerful and less expected.
There was a problem hiding this comment.
I thought that a -> b -> Ordering is somewhat unexpected. Anyway I changed specializations to a -> b -> Ordering
There was a problem hiding this comment.
Could you please do the same for eqBy?
There was a problem hiding this comment.
Done. Thank you for noticing things.
fdd57ea to
586d71c
Compare
586d71c to
e48fe57
Compare
3 participants
Progress is tracked in #299