Add PolyKinds to Clash.Class.BitPack.Internal
#2993
Conversation
This gives us `BitPack` instances for `Proxy a` where `a` is a Kind from our derived `BitPack` instance for `Proxy a`
lmbollen
force-pushed
the
lucas/proxy-bitpack-instance
branch
from
a749521 to
b659c7e
Compare
|
This is the second time that we've been really confused by |
There was a problem hiding this comment.
I don't think a commit titled Add regression doctests for BitPack (Proxy a) should also change the definition of the instance without explaining why. Likewise, the whole PR and PR cover letter need to mention this change.
Your commit message for the other commit more clearly describes the change than the PR cover letter. How about changing the PR cover letter to:
This gives us
BitPackinstances forProxy awhereais a Kind from our derivedBitPackinstance forProxy a. Examples areProxy 42,Proxy (n :: Nat)orKnownNat n => Proxy n.This should have been added in #2990
This PR also corrects a mistake in the
BitPackinstance definition, which was not intended to have any constraints.This should also be backported to 1.8, see #2991
lmbollen
force-pushed
the
lucas/proxy-bitpack-instance
branch
from
b659c7e to
1c193ad
Compare
There was a problem hiding this comment.
As I indiicated in PR #2994, I'm confused by the structure you're going for. Why make a PR on your topic branch instead of just including the default extension here or closing this and making a new PR with a new title?
| @@ -12,7 +12,7 @@ Maintainer : QBayLogic B.V. <devops@qbaylogic.com> | |||
| {-# LANGUAGE TemplateHaskell #-} | |||
| {-# LANGUAGE TypeFamilies #-} | |||
| {-# LANGUAGE UndecidableInstances #-} | |||
|
|
|||
| {-# LANGUAGE PolyKinds #-} -- Required for BitPack (KnownNat n) => BitPack (Proxy n) instances | |||
There was a problem hiding this comment.
Sorry I missed this on initial review, but what does BitPack (KnownNat n) mean? I initially mentally autocorrected this to KnownNat n => BitPack (Proxy n) which makes sense although it is overly specific: it forces n to be a Nat which needs PolyKinds. How about:
| {-# LANGUAGE PolyKinds #-} -- Required for BitPack (KnownNat n) => BitPack (Proxy n) instances | |
| {-# LANGUAGE PolyKinds #-} -- Required for BitPack (Proxy (n :: Nat)) instances |
It's still overly specific, but at least it makes sense. I don't think a BitPack (KnownNat n) constraint means anything?
This gives us BitPack instances for
Proxy awhereais a Kind from our derivedBitPackinstance forProxy a. Examples areProxy 42, Proxy (n :: Nat)orKnownNat n => Proxy n.BitPackinstance forData.Proxy.Proxy#2990BitPackinstance forData.Proxy.Proxy(copy #2990) #2991