-
Notifications
You must be signed in to change notification settings - Fork 15
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
feat(air)!: change fold over canon map iterator contents [fixes VM-620] #843
Conversation
true | ||
} | ||
} else { | ||
false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think complying with filter
interface is too much hassle here.
Try to rewrite it with a simple for loop (pushing to a Vec) to see if it shorter and clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replaced. The suggested approach delivers a smaller nibble for sure.
It worth adding a small tests that just demonstrates specific bit of functionality, e.g. |
JFYI I altered the existing test to test duplicates iterating with fold over SCM. |
|
||
for val in canon_stream_map.canon_stream_map.iter().rev() { | ||
if let Some(map_key) = StreamMapKey::from_kvpair_owned(val) { | ||
if met_keys.get(&map_key).is_none() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just !met_keys.contains(&map_key)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch
|
||
for val in canon_stream_map.canon_stream_map.iter().rev() { | ||
if let Some(map_key) = StreamMapKey::from_kvpair_owned(val) { | ||
if !met_keys.contains(&map_key) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be slightly more performant with the Entry API (no double hashing and lookup), but I'm not sure it worth it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK
This is to align the spec with the implementation. The spec tells
fold
iterates over unique(conflicts are resolved with LastWriteWin policy) kv pairs. The implementation violates this delivering all kv pairs together with duplicates.