-
Notifications
You must be signed in to change notification settings - Fork 57
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
Signal's max
parameter isn't a max
#196
Comments
This isn't possible. Imagine a situation where code is ported from oMigen to nMigen. This means that, for some time, code in nMigen and code in nMigen compat mode (which behaves ~exactly like oMigen) must coexist. Consider this situation: would you like to have to scroll up and see which Also, there's alerady nontrivial amount of nMigen code, so changing this now would be breaking all of it, and moreover, as you see in README, the API of However, what is possible is deprecating /cc @emilazy |
Those semantics are the same as Python's |
Yes. The problem isn't so much semantics as the name; it says |
Alright, then OK to add and recommend start/stop and still support min/max for legacy reasons. |
I think it should be a separate constructor, similar to |
Yeah, changing the meaning of max is problematic and would break backward compatibility. I'm in favor of a new constructor/builder. |
Agreed, I'd much rather Signal had a few constructors with simple signatures than the current monolith. |
According to the docs:
But the maximum value of a signal is by definition inclusive for discrete ranges. I'd prefer that
max
be changed to inclusive, defaults to 1, and all uses ofmax
instead usemax+1
.The text was updated successfully, but these errors were encountered: