Skip to content

Fix Monoid / SemiGroup instances for GHC 8.4#7

Merged
conal merged 1 commit intoconal:masterfrom
guibou:fix_semigroup_instance
Jul 16, 2018
Merged

Fix Monoid / SemiGroup instances for GHC 8.4#7
conal merged 1 commit intoconal:masterfrom
guibou:fix_semigroup_instance

Conversation

@guibou
Copy link
Copy Markdown

@guibou guibou commented Jul 16, 2018

Build tested with GHC 8.0.2, 8.2.2 and 8.4.3.

Build tested with GHC 8.0.2, 8.2.2 and 8.4.3.
@conal conal merged commit 88a73d0 into conal:master Jul 16, 2018
@conal
Copy link
Copy Markdown
Owner

conal commented Jul 16, 2018

Thanks. With GHC 8.0.2, I got some warnings for missing mappend definitions, so I defined them to be equal to (<>). Expected? Is there not a default mappend = (<>)?

@guibou
Copy link
Copy Markdown
Author

guibou commented Aug 23, 2018

https://prime.haskell.org/wiki/Libraries/Proposals/SemigroupMonoid is saying

#if !(MIN_VERSION_base(4,11,0))
  -- this is redundant starting with base-4.11 / GHC 8.4
  -- if you want to avoid CPP, you can define `mappend = (<>)` unconditionally
  mappend = (<>)
#endif

So, apparently:

  • GHC 8.0: Semigroup and Monoid are not related. To implement Monoid you need to provide mappend and mempty. Adding mappend = (<>) is correct. There is no default.
  • GHC 8.4: Semigroup is a base class of Monoid: To implement Monoid you need to provide only mempty. <> comes from Semigroup. However Monoid keeps mappend which is implemented by default with <>. Implementing it is correct.

So as far as I know, if you want to keep support for GHC 8.0, you need mappend = (<>). So you are correct.

Sorry I missed that in my PR. And I apologie I missed your comment and took one month to answer.

@conal
Copy link
Copy Markdown
Owner

conal commented Aug 30, 2018

Thanks very much for the explanations. I'm just catching up after a very distracted week. Released to Hackage.

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

Successfully merging this pull request may close these issues.

2 participants