-
Notifications
You must be signed in to change notification settings - Fork 58
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Reconsider Signal.range() and Signal.enum() #225
Comments
I like the idea of |
I also dislike I personally think having |
|
(here we go...) |
@RobertBaruch The problem is that eventually someone tries to make a signal one bit shorter than x bits, and writes |
I personally like all factories, all the time. It's more readable to me than trying to guess what |
That's reasonable, but what do we do with |
Records should be described with dictionaries rather than opaque tuples anyway. |
That's not making it any better given that |
Shapes have long been a part of nMigen, but represented using tuples. This commit adds a Shape class (using namedtuple for backwards compatibility), and accepts anything castable to Shape (including enums, ranges, etc) anywhere a tuple was accepted previously. In addition, `signed(n)` and `unsigned(n)` are added as aliases for `Shape(n, signed=True)` and `Shape(n, signed=False)`, transforming code such as `Signal((8, True))` to `Signal(signed(8))`. These aliases are also included in prelude. Preparation for #225.
We currently have three ways to specify the shape of a signal using
Record
/Layout
:int
,(int, bool)
,EnumSubclass
.We currently have four ways to specify the shape of a signal using
Signal
:int
,(int, bool)
(both with theSignal()
constructor),EnumSubclass
(with theSignal.enum()
constructor),range
(with theSignal.range()
constructor).I think that in light of this, introducing separate
enum
andrange
constructors might have been a mistake. It should be possible to unambiguously fold these back intoSignal
. The initial decision was made because having bothshape
andmin
/max
is ambiguous, but dispatching over the type ofshape
is not ambiguous and covers all of these use cases well.The text was updated successfully, but these errors were encountered: