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

Force all "These" constructors created by Align instances? #32

Open
isomorphism opened this issue Sep 14, 2015 · 3 comments
Open

Force all "These" constructors created by Align instances? #32

isomorphism opened this issue Sep 14, 2015 · 3 comments

Comments

@isomorphism
Copy link
Contributor

Based on the conversation on pull request #27 I'm wondering if it ever makes sense to leave the These constructors introduced by an Align instance unevaluated, since the reasoning involved isn't specific to HashMap at all.

At very least we should probably use the strict version for Data.Map and Data.IntMap, which have "strict" modules with a shared base type like HashMap does. Pull request #29 from @phadej covers the first, but not the second.

For other types we'd have to seq the results manually, but other than cluttering the code up slightly it seems strictly (heh) better than creating more thunks.

@phadej
Copy link
Collaborator

phadej commented Sep 14, 2015

I can amend #29 with IntMap case too.

I don't really strong opinion about others, as it isn't required by data structures "promises". I cannot come with the way to break seqd versions though.

EDIT it isn't even an API breaking change.

@isomorphism
Copy link
Contributor Author

If you feel like it, go ahead and add IntMap and commit the change. If not, I'll take care of it later.

I'm gonna think a bit more about the other instances before changing some/all of them. I'm nearly certain it's safe if the only potential bottoms are the elements in the data structures, but not sure about the scenario where the "spine" of an input contains bottoms.

@isomorphism
Copy link
Contributor Author

I think I'm deciding I don't feel like deciding about this right now. The effort of convincing myself that forcing it is correct in all cases is probably greater than any benefit it would have.

I still think it's probably the Right Thing(tm) so I'll leave this issue open for now in case someone else wants to think about it.

But for now, the changes that have already been merged are enough.

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

No branches or pull requests

2 participants