generalize filterM, mapAndUnzipM, zipWithM, zipWithM_, replicateM, replicateM_
ClosedPublic

Authored by strake on Oct 13 2015, 9:37 PM.

Details

Summary

NB: This can lead to performance regressions if (*>) is less optimised than (>>) (and/or if traverse is worse than sequence).

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
strake updated this revision to Diff 4492.Oct 13 2015, 9:37 PM
strake retitled this revision from to generalize filterM, mapAndUnzipM, zipWithM, zipWithM_, replicateM, replicateM_.
strake updated this object.
strake edited the test plan for this revision. (Show Details)
strake set the repository for this revision to rGHC Glasgow Haskell Compiler.
strake added a project: GHC.
strake updated the Trac tickets for this revision.
hvr requested changes to this revision.EditedOct 14 2015, 12:44 AM
hvr edited edge metadata.

LGTM, but needs an changelog.md entry

we also should do a nofib run to make sure there's no unexpected changes in allocations (although I'm not sure if nofib actually exercises these verbs)

This revision now requires changes to proceed.Oct 14 2015, 12:44 AM
strake updated this revision to Diff 4947.Nov 5 2015, 11:15 PM
strake edited edge metadata.

changelog

bgamari edited edge metadata.Nov 7 2015, 5:26 PM

Assuming this is in line with the CLC's vision this looks good to me.

hvr accepted this revision.Nov 8 2015, 1:15 AM
hvr edited edge metadata.

same response as @bgamari

This revision is now accepted and ready to land.Nov 8 2015, 1:15 AM
hvr requested changes to this revision.EditedNov 8 2015, 1:16 AM
hvr edited edge metadata.

better un-accept to make sure this isn't merged before an official CLC stamp of approval

PS: @glguy reminded me there's a risk of regression until (>>) = (*>) holds, so there may be a reason to hold off on this

This revision now requires changes to proceed.Nov 8 2015, 1:16 AM
hvr added a subscriber: glguy.Nov 8 2015, 1:54 AM
ekmett accepted this revision.Nov 10 2015, 6:57 AM
ekmett added a reviewer: ekmett.
ekmett added a subscriber: ekmett.

CLC Approved.

It seems worth including a note in the documentation about 8.0 that indicates that if you experience any performance regressions due to these generalizations, you should fix the implementation of (*>) to match your implementation of (>>).

Then these changes can be done with a clean conscience.

hvr updated this object.Nov 10 2015, 7:07 AM
hvr edited edge metadata.
hvr updated this object.
hvr resigned from this revision.Nov 10 2015, 7:09 AM
hvr removed a reviewer: hvr.

(resigning to get out of the way)

This revision is now accepted and ready to land.Nov 10 2015, 7:09 AM
In D1324#42233, @ekmett wrote:

It seems worth including a note in the documentation about 8.0 that indicates that if you experience any performance regressions due to these generalizations, you should fix the implementation of (*>) to match your implementation of (>>).

Where, in docs/users_guide/7.12.1-notes.rst?

bgamari requested changes to this revision.Nov 11 2015, 5:34 AM
bgamari edited edge metadata.
In D1324#42246, @strake wrote:
In D1324#42233, @ekmett wrote:

It seems worth including a note in the documentation about 8.0 that indicates that if you experience any performance regressions due to these generalizations, you should fix the implementation of (*>) to match your implementation of (>>).

Where, in docs/users_guide/7.12.1-notes.rst?

Sounds like a fine place.

Also, do you think you could rebase this and perhaps move the base/changelog entry up the document. It would be nice to have all of the entries for Applicative generalizations grouped together.

This revision now requires changes to proceed.Nov 11 2015, 5:34 AM
strake updated this revision to Diff 5034.Nov 11 2015, 9:14 AM
strake edited edge metadata.

Also, do you think you could rebase this and perhaps move the base/changelog entry up the document. It would be nice to have all of the entries for Applicative generalizations grouped together.

Yes, done.

bgamari requested changes to this revision.Nov 11 2015, 9:37 AM
bgamari edited edge metadata.

@strake, it seems there are some warnings still.

This revision now requires changes to proceed.Nov 11 2015, 9:37 AM
strake updated this revision to Diff 5035.Nov 11 2015, 9:42 AM
strake edited edge metadata.

@strake, it seems there are some warnings still.

No more

bgamari accepted this revision.Nov 15 2015, 5:03 AM
bgamari edited edge metadata.

Thanks @strake!

This revision is now accepted and ready to land.Nov 15 2015, 5:03 AM
This revision was automatically updated to reflect the committed changes.