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

Data.Map.Strict.mergeWithKey does not force the result of the combining function #1022

Closed
meooow25 opened this issue Aug 11, 2024 · 5 comments · Fixed by #1024
Closed

Data.Map.Strict.mergeWithKey does not force the result of the combining function #1022

meooow25 opened this issue Aug 11, 2024 · 5 comments · Fixed by #1024

Comments

@meooow25
Copy link
Contributor

mergeWithKey should force x':

Just x2 -> case f kx x x2 of
Nothing -> link2 l' r'
Just x' -> link kx x' l' r'

Found in #1021


IntMap seems fine

combine = \(Tip k1 x1) (Tip _k2 x2) -> case f k1 x1 x2 of Nothing -> Nil
Just !x -> Tip k1 x

@treeowl
Copy link
Contributor

treeowl commented Aug 11, 2024

Good catch. I wonder if that's been there forever or if I accidentally introduced it with the merge algorithm change.... If it was my fault, I'm sorry. Fortunately, almost no one uses this hideous function.

@meooow25
Copy link
Contributor Author

Looks like it's been there since the function was introduced.

Fortunately, almost no one uses this hideous function.

We can fix it now, but do we plan to remove it at some point?

@treeowl
Copy link
Contributor

treeowl commented Aug 11, 2024

I tried a few years ago, but one or two people opposed it.

@meooow25
Copy link
Contributor Author

Oh well, at least it has good documentation explaining how it is unsafe.

@treeowl
Copy link
Contributor

treeowl commented Aug 11, 2024

We should also document that the function doesn't crack open the replacement trees to force values within.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants