Make Semigroup a superclass of Monoid
ClosedPublic

Authored by hvr on Sep 7 2017, 3:46 AM.

Details

Summary

Unfortunately, this requires introducing a couple of .hs-boot files to
break up import cycles (mostly to provide class & typenames in order to
be able to write type signatures).

This does not yet re-export (<>) from Prelude (while the class-name
Semigroup is reexported); that will happen in a future commit.

Test Plan

local ./validate passed

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.
hvr created this revision.Sep 7 2017, 3:46 AM
ekmett accepted this revision.Sep 7 2017, 4:00 AM
This revision is now accepted and ready to land.Sep 7 2017, 4:00 AM
hvr updated this revision to Diff 13774.Sep 7 2017, 7:15 AM
  • Fix T6117 compilation
hvr updated this revision to Diff 13775.Sep 7 2017, 9:13 AM
  • Recenter T12234(optasm) allocation perf numbers
RyanGlScott added inline comments.
libraries/base/GHC/Base.hs
421–422

Perhaps unrelated to this patch, but does the SMP have a plan to turn the existing Monoid a => Monoid (Maybe a) instance to Semigroup a => Monoid (Maybe a)?

hvr added inline comments.Sep 7 2017, 12:47 PM
libraries/base/GHC/Base.hs
421–422

Personally I think it makes sense, but I hesitated to make this change just yet,
as I liked to avoid sneaking in generalisations into this specific patch which was already a bit of a struggle to get to the point of passing validate :-)

And I'd like such a change to have its own commit and separately signed off by the CLC... (hint, hint) :-)

RyanGlScott requested changes to this revision.Sep 7 2017, 12:54 PM

Since you asked for a review, here's some things:

  • We definitely need some additions to the base changelog describing what changed (it public-facing modules, at least).

Another inline comment below.

libraries/base/Data/Semigroup/Internal.hs
8

If I'm reading this correctly, this contains definitions/datatypes that are used across both Data.Semigroup and Data.Monoid, correct? If so, it might be a good idea to document this fact.

libraries/base/GHC/Base.hs
421–422

Fair enough.

This revision now requires changes to proceed.Sep 7 2017, 12:54 PM
hvr added a comment.Sep 7 2017, 1:36 PM

Since you asked for a review, here's some things:

  • We definitely need some additions to the base changelog describing what changed (it public-facing modules, at least).

The thing is, that this diff doesn't complete phase 2 yet; hence why I didn't yet write a changelog entry, which I'd have to revise otherwise later.

Moreover, there'll be a couple of follow-up commits anyway once this patch has finally landed...

If you insist, I can write a partial changelog entry now... :-)

libraries/base/Data/Semigroup/Internal.hs
8

Good catch; yes this module is mostly here to disentangle the import graph a bit. I'll add a note

In D3927#110161, @hvr wrote:

If you insist, I can write a partial changelog entry now... :-)

Please do! At the very least, it's a nice way to summarize what changed in this particular commit. That way, I can look at this commitdiff later and remind myself what transpired.

hvr updated this revision to Diff 13780.Sep 7 2017, 1:58 PM
hvr edited edge metadata.
  • changelog entry & module header [skip ci]
This revision was automatically updated to reflect the committed changes.