Strongly separate clash-protocols-base and clash-protocols
#118
Conversation
lmbollen
force-pushed
the
lucas/reorder-modules
branch
from
f09f91b to
5ddd411
Compare
|
Unused import in It would be nice if we could completely hide the |
Why is that? Doesn't the in |
lmbollen
force-pushed
the
lucas/reorder-modules
branch
2 times, most recently
from
f082174 to
db1274d
Compare
lmbollen
force-pushed
the
lucas/reorder-modules
branch
from
db1274d to
2bda9fb
Compare
I think if you build the package from github using e.g.: You also need to add the dependency for |
|
Right, I had misunderstood the comment. |
There was a problem hiding this comment.
This are no longer needed:
clash-protocols-base/src/Protocols/Plugin.hs:
{-# LANGUAGE OverloadedStrings #-}
{-# LANGUAGE PatternSynonyms #-}This can now be guarded again:
clash-protocols/src/Protocols/Internal.hs:
#if !MIN_VERSION_clash_prelude(1, 8, 2)
{-# OPTIONS_GHC -fno-warn-orphans #-}
#endifAnd I'd like to suggest to export everything from Protocols.Plugin{,.Units,.TaggedBundle} from Protocols.Internal. This makes things easier and less surprising.
I did turn that suggestion into individual per-file suggestions, but if you want, I can add a fixup commit that does this, since I already wrote it to test it. Saves you the work of recreating it.
Also, I only commented on documentation things if I thought they were easy to miss. All obvious "polishing the docs" stuff goes without comment, we can do it later.
| {- | __NB__: The documentation only shows instances up to /3/-tuples. By | ||
| default, instances up to and including /12/-tuples will exist. If the flag | ||
| @large-tuples@ is set instances up to the GHC imposed limit will exist. The | ||
| GHC imposed limit is either 62 or 64 depending on the GHC version. | ||
| -} |
There was a problem hiding this comment.
This is slightly misleading. People will encounter this documentation in clash-protocols. So they might set the -flarge-tuples flag on clash-protocols when really they should set it on both clash-protocols-base as well as clash-protocols.
However, I now see it's actually worse than that. The instances defined in clash-protocols-base are not documented at all in the Haddock for clash-protocols. I think this is a real shortcoming. People will not easily be able to know that these instances exist at all.
| import Protocols.Internal | ||
| import Protocols.Internal.TH (idleCircuitTupleInstances) | ||
| import Protocols.Plugin | ||
| import Protocols.Plugin.Cpp (maxTupleSize) |
There was a problem hiding this comment.
As you can see in the generated Haddock, this does not work. maxTupleSize is supposed to be 3 when generating Haddock. But when generating Haddock for clash-protocols, clash-protocols-base is compiled without -DHADDOCK_ONLY and so maxTupleSize is 12.
This package will need its own copy of Cpp.hs. That will also solve that clash-protocols will once again honour its own -flarge-tuples flag instead of the one on clash-protocols-base.
`clash-protocols-base` should only contain code and definitions related to the circuit plugin. This includes the `Circuit` definition and `Protocol` typeclass. Furthermore we include instances for types imported from underlying clash libraries such as tuples, `Vec` and `Signal`.
It not only contains classes, but also types
…e to separate modules
The separation between plugin related code and protocol related code should be obvious from the module hierarchy
DigitalBrains1
force-pushed
the
lucas/reorder-modules
branch
from
c304653 to
34d51cb
Compare
clash-protocols-baseshould only contain code and definitions related to the circuit plugin. This includes theCircuitdefinition andProtocoltypeclass. Furthermore we include instances for types imported from underlying clash libraries such as tuples,VecandSignal.