-
Notifications
You must be signed in to change notification settings - Fork 174
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
[RFC] Redesign UserValue to avoid breaking code that inherits from it #355
Comments
I mostly agree on what you wrote here, however, I have the concern, that when there is no common base class for nMigen values, it would be harder to check which fields of an object are somehow nMigen related (e.g. for implementing iter_flat in a more generic way that doesn't assume Record is the only signal bundle) |
Well, in principle, instead of
... my concern is that doing this is not a very good idea because there is no way to write an In other words, you can do it quite easily even after this change, but I think the minor speed bump you get is a good reminder to ask yourself, "is this actually a good idea?" |
okay |
I prefer |
@awygle We have a consensus then. |
Additional wishes:
|
Per discussion in IRC meeting on July 7th 2020:
|
@awygle Thanks a lot for summarizing this! Would you be up for implementing it, too? |
Sure. Post-0.3, I assume? |
I'd prefer to add ValueCastable in 0.3, deprecate UserValue in 0.3, and then finally pull the plug on UserValue in 0.4. |
Notably this doesn't actually prevent us from keeping Record beyond 0.4 because Record can be redesigned to keep compat with (broken) UserValue design by redefining |
UserValue
fulfills an important role: it allows code downstream from nMigen to define objects that can be fluently used in nMigen code, i.e. on RHS and, possibly, LHS of nMigen assignments. It was originally created to reflect the non-primitive nature of Records, and also because there was some disagreement on the best way to handle Records. These disagreements have persisted, and I both acknowledge them, and seeUserValue
as an important tool to explore any solutions.Unfortunately, as currently implemented, it has a fatal flaw. It pollutes the namespace of the classes that inherit from it. We add new methods to
Value
liberally (see #352 for the latest addition), and each of these methods has the potential of clashing with downstream code. As it is, any new method onValue
is a breaking change, which is not acceptable.Fortunately, there is a good solution. One can observe that
UserValue
s (for example, anyRecord
s--at least post-#354) do not really need to implement any numeric operations because they are not numbers--indeed, if they were simple numbers, they would be normalValue
s. They don't necessarily implement bit vector operations either--for example,Record
overrides__getitem__
. They only need to be implicitly castable toValue
as opposed to be a kind ofValue
.I propose that we change
UserValue
to not inherit fromValue
at all, and make sure it injects a small and well-defined set of attributes into derived classes. Its primary purpose would then to be a marker class thatValue.cast
uses to recognize that it should first lower its argument.Unresolved questions:
lower
toas_value
? The former is a fairly generic term, and therefore potentially useful to derived classes. The latter, in context of nMigen, is quite unambiguous, and very unlikely to clash with user-defined record fields.eq
? SomeUserValue
s, such asRecord
s, will be commonly used on LHS, and writingrec1.as_value().eq(rec2)
is a lot less convenient thanrec1.eq(rec2)
. However, one can implement aUserValue
that lowers to a constant, and that would be never usable on LHS.One option is to leave it to the specific derived class to implement
eq
, since the implementation is as simple asreturn Assign(self, value, src_loc_at=1)
. Such an approach could also be useful because the derived class might be able to implement useful diagnostics that are not applicable toValue
s in general.The text was updated successfully, but these errors were encountered: