Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Reconsider Signal.range() and Signal.enum() #225

Closed
whitequark opened this issue Sep 22, 2019 · 9 comments
Closed

Reconsider Signal.range() and Signal.enum() #225

whitequark opened this issue Sep 22, 2019 · 9 comments
Labels
improvement
Milestone
0.1

Comments

@whitequark
Copy link
Contributor

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 the Signal() constructor), EnumSubclass (with the Signal.enum() constructor), range (with the Signal.range() constructor).

I think that in light of this, introducing separate enum and range constructors might have been a mistake. It should be possible to unambiguously fold these back into Signal. The initial decision was made because having both shape and min/max is ambiguous, but dispatching over the type of shape is not ambiguous and covers all of these use cases well.
@whitequark whitequark added this to the 0.1 milestone Sep 22, 2019
@emilazy
Copy link
Contributor

emilazy commented Sep 22, 2019 •
edited

I like the idea of Signal(8), Signal(range(..., ...)) and Signal(MyEnum), but I really dislike Signal((16, True)) and would only prefer this reform if a better alternative for that can be found (otherwise I'd rather have a Signal.signed and move it all to constructors).

@whitequark
Copy link
Contributor Author

whitequark commented Sep 22, 2019 •
edited

I also dislike Signal((16, True)). It is OK that Signal() accepts this tuple, since this is what Value.shape() returns, and having Signal(value.shape()) be valid is important, but it is a bad user interface.

I personally think having Signal(8), Signal(range(..., ...)), Signal(MyEnum) and Signal.signed(8), with no other constructors, is a reasonable compromise, since adding signed() to prelude for this single use case is not acceptable. Signal.signed(x) could be documented as a readable alias for Signal((x, True)).

@RobertBaruch
Copy link

Signal(-8) == Signal.signed(8)? :D

@emilazy
Copy link
Contributor

emilazy commented Sep 22, 2019

(here we go...)

@whitequark
Copy link
Contributor Author

@RobertBaruch The problem is that eventually someone tries to make a signal one bit shorter than x bits, and writes Signal(x-1), and if x is zero, this does something totally unexpected instead of erroring out. So I don't think that's acceptable.

@RobertBaruch
Copy link

RobertBaruch commented Sep 22, 2019 •
edited

I personally like all factories, all the time. It's more readable to me than trying to guess what __init__ does.

@whitequark
Copy link
Contributor Author

That's reasonable, but what do we do with Record?

@emilazy
Copy link
Contributor

emilazy commented Sep 22, 2019

Records should be described with dictionaries rather than opaque tuples anyway.

@whitequark
Copy link
Contributor Author

That's not making it any better given that (shape, direction) is still a tuple.

whitequark added a commit that referenced this issue Oct 11, 2019
hdl.ast: Value.{wrap→cast}
d72d4a5 
Preparation for #225.
whitequark added a commit that referenced this issue Oct 11, 2019
hdl.ast: add an explicit Shape class, included in prelude.
6aabdc0 
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.
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
improvement
Projects
None yet
Milestone
0.1
Development

No branches or pull requests
3 participants
@whitequark @RobertBaruch @emilazy