fhdl/structure: add check for equality for _Slice #292
Conversation
|
@sbourdeauducq you may want to take a look |
|
@m-labs please take a look |
|
See the comment above the code you modified. At the very least it would need to be updated. But what is the rationale for this change?
|
maass-hamburg
force-pushed
the
structure_equal_check_slice
branch
from
1a21b4d to
b25cc2e
Compare
|
@sbourdeauducq the reason for that change is demonstated here: enjoy-digital/litex@d63d8d9. There oe1 and oe2 could be Slices and without this change, we would get a Error. |
migen/fhdl/structure.py
Outdated
| if isinstance(self, _Operator) and self.op == "==": | ||
| a, b = self.operands | ||
| if isinstance(a, Constant) and isinstance(b, Constant): | ||
| return a.value == b.value | ||
| if isinstance(a, Signal) and isinstance(b, Signal): | ||
| return a is b | ||
| if isinstance(a, _Slice) and isinstance(b, _Slice): | ||
| return (a.value is b.value) and (a.start == b.start) and (a.stop == b.stop) |
There was a problem hiding this comment.
Shouldn't it be a.value == b.value ? You could in theory slice a Constant, or slice a slice.
maass-hamburg
force-pushed
the
structure_equal_check_slice
branch
from
b25cc2e to
3db32f7
Compare
migen/fhdl/structure.py
Outdated
| if isinstance(self, _Operator) and self.op == "==": | ||
| a, b = self.operands | ||
| if isinstance(a, Constant) and isinstance(b, Constant): | ||
| return a.value == b.value | ||
| if isinstance(a, Signal) and isinstance(b, Signal): | ||
| return a is b | ||
| if isinstance(a, _Slice) and isinstance(b, _Slice): | ||
| return (a.value == b.value).__bool__ and (a.start == b.start) and (a.stop == b.stop) |
There was a problem hiding this comment.
without that we are returning a _Operator object. With that we are getting a boolean value. Would have used bool(a.value == b.value) and just (a.value == b.value). But these don't work
There was a problem hiding this comment.
.__bool__ is a function which just always evaluates to True according to Pythons lax rules. This is not working either. Wat issue do you have with just (a.value == b.value)? The recursive call should just resolve to is when it's a Signal.
There was a problem hiding this comment.
(a.value == b.value) would just return an object of type _Operator. bool(a.value == b.value) now works, it forces it to resolve.
maass-hamburg
force-pushed
the
structure_equal_check_slice
branch
from
3db32f7 to
7845b72
Compare
migen/fhdl/structure.py
Outdated
| @@ -29,12 +29,16 @@ class _Value(DUID): | |||
| def __bool__(self): | |||
| # Special case: Constants and Signals are part of a set or used as | |||
| # dictionary keys, and Python needs to check for equality. | |||
| # The part for the Slice has been added to do the same for Slices of | |||
| # Signals, Constants, and Slices. | |||
There was a problem hiding this comment.
Why not just edit the comment to say "Constants, Signals and Slices" ?
Also this slice comparison is a bit janky, for instance s == s[:] evaluates to False even though they are the same.
maass-hamburg
force-pushed
the
structure_equal_check_slice
branch
from
7845b72 to
85321af
Compare
migen/fhdl/structure.py
Outdated
| if isinstance(a, _Slice) and (isinstance(b, Constant) or isinstance(b, Signal)): | ||
| return (bool(a.value == b) and (a.start == 0) and (a.stop == len(b))) | ||
| if isinstance(b, _Slice) and (isinstance(a, Constant) or isinstance(a, Signal)): | ||
| return (bool(b.value == a) and (b.start == 0) and (b.stop == len(a))) |
There was a problem hiding this comment.
Yeah well this does not cover all cases, does it?
I was not saying slice comparison should be implemented and implemented fully, I'm just pointing out that it's not easy. A careful decision needs to be made.
There was a problem hiding this comment.
I think it covers all cases. if one is a slice and the other is a Constant or a Signal. The slice can even be a slice of a slice and so on.
There was a problem hiding this comment.
Those work?
s[:][:] == s
s[1:2][1:2] == s[2]
There was a problem hiding this comment.
s[:][:] == s
this should
s[1:2][1:2] == s[2]
this should raise a error as s[1:2] would have a len of 1 and for the second [1:2] it would need to have a len of 2.
But s[1:2][0:1] == s[2] should work and return `false
There was a problem hiding this comment.
How about s[1:3][1:2] == s[2]?
There was a problem hiding this comment.
I rewrote it, so it works with s[1:3][1:2] == s[2].
add check for equality for _Slice. Signed-off-by: Fin Maaß <[email protected]>
maass-hamburg
force-pushed
the
structure_equal_check_slice
branch
from
85321af to
a950eb8
Compare
|
@sbourdeauducq I rewrote it, it should now cover all cases with slices. It now resolves each slice until it is no longer a slice and than compares them. |
add check for equality for _Slice.