containers icon indicating copy to clipboard operation
containers copied to clipboard

Published 20 hours ago • verified publisher icon haskell

Fix -incomplete-uni-patterns

Open ckoparkar opened this issue 6 years ago • 11 comments

#590.

ckoparkar avatar Jan 16 '19 23:01 ckoparkar

@treeowl ping :)

ckoparkar avatar Jan 21 '19 11:01 ckoparkar

Have you tried comparing the merge vs. mergeWithKey' approaches?

Oops, I thought the plan was to apply this simple case+error solution before that. I'll try out the merge vs. mergeWithKey' idea and update this patch in the next couple days.

[Changes planned]

ckoparkar avatar Jan 21 '19 18:01 ckoparkar

I would like to make a release in the next week or so, now that #593 is fixed, but we can certainly wait a couple days.

treeowl avatar Jan 23 '19 01:01 treeowl

Hi @treeowl, I wanted to get this done this week, but it didn't happen. I spent a little bit of time trying to understand various parts of the Data.Sequence implementation. I'll finish it soon, over the weekend perhaps, but please don't let this block a release :)

ckoparkar avatar Feb 02 '19 13:02 ckoparkar

Oops, I didn't make a release as intended.... Can you rebase this and look into my question?

treeowl avatar Apr 12 '19 18:04 treeowl

@treeowl sorry for squatting on this for so long. I'll pick this up and try to finish it by this weekend.

ckoparkar avatar Apr 15 '19 19:04 ckoparkar

@ckoparkar Ping!

mitchellwrosen avatar Jun 21 '19 17:06 mitchellwrosen

My impression is that there's very little left to do before this could be merged. Or maybe we could at least merge the non-controversial bits and leave out the "merge vs. mergeWithKey'" "controversy". (I have no idea what it's about.)

The PR definitely needs a rebase though.

@ckoparkar Can you bring this to completion?

Or would you, @mitchellwrosen, possibly volunteer to finish this?

sjakobi avatar Jul 17 '20 16:07 sjakobi

@sjakobi I'm afraid I've forgotten most of the context here. As far as I can tell, David's suggestion was to try to use merge instead of mergeWithKey' in some of these places. Sorry, but I won't be able to contribute much here. If you or someone else wants to pick this up, please feel free.

ckoparkar avatar Jul 24 '20 15:07 ckoparkar

mergeWithKey is a nasty API that basically forces users to supply partial functions and also allows the data structure invariants to be violated. merge is slightly less general API that nevertheless supports pretty much everything one might actually want from mergeWithKey. If we can use it without losing performance, that would be a good way to be rid of some incomplete patterns.

treeowl avatar Jul 24 '20 15:07 treeowl

Thanks for the update, @ckoparkar, and thanks for the explanation, @treeowl! :)

I generally agree that it would be nice to use merge instead of mergeWithKey'. It's far from obvious to me though, that this could be done here without loss of performance: IntMap.merge is just runIdentity (mergeA ...) and mergeA has to do quite a bit of extra work to get the effects sequencing right. mergeWithKey' also offers a small optimization specifically for unionWithKey:

https://github.com/haskell/containers/blob/4ac960a01f058ae923cf51c6108aae0acbb802dd/containers/src/Data/IntMap/Internal.hs#L1372-L1375

It would of course still be interesting to benchmark results using merge, but I think the chances that the results would be good are fairly low.

I also kind of feel that contributors shouldn't be expected to solve a different problem than they originally intended to work on.

So, what I see that needs to be done for this PR to be accepted is:

  1. A rebase.
  2. Remove the outdated comments in mergeWithKey that @treeowl pointed out above.

sjakobi avatar Jul 26 '20 22:07 sjakobi