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.

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

BitSequence comparison issue #253

Open
addinh opened this issue Jun 18, 2021 · 1 comment
Open

BitSequence comparison issue #253

addinh opened this issue Jun 18, 2021 · 1 comment
Labels
bug Confirmed/reproduced bug

Comments

@addinh
Copy link

addinh commented Jun 18, 2021

It seems the comparison functions for BitSequence may be erroneous? There are nots in the functions for <=, <, >, >= that shouldn't be there, unless I'm understanding the purpose of comparison wrong

pyftdi/pyftdi/bits.py

Lines 234 to 250 in 2ad8a1a

def __eq__(self, other):
return self._cmp(other) == 0
def __ne__(self, other):
return not self == other
def __le__(self, other):
return not self._cmp(other) <= 0
def __lt__(self, other):
return not self._cmp(other) < 0
def __ge__(self, other):
return not self._cmp(other) >= 0
def __gt__(self, other):
return not self._cmp(other) > 0

Adding to this, the base function _cmp(self, other) also seems odd. I'm assuming the comparison is based on int value, but the function compares length first (which would fail in an example case: 101 vs 0010), then compares bit by bit. However, n from enumerate should only have values from 0 to len(self)-1. This makes the function less than always return True if both BitSequences have the same length.

pyftdi/pyftdi/bits.py

Lines 252 to 260 in 2ad8a1a

def _cmp(self, other):
# the bit sequence should be of the same length
ld = len(self) - len(other)
if ld:
return ld
for n, (x, y) in enumerate(zip(self._seq, other.sequence()), start=1):
if xor(x, y):
return n
return 0

A simplier way to compare would be to convert to int and compare directly, for example int(self) and int(other). Again, I may be understanding the purpose wrong, but if comparison deals with the int values of BitSequences then using the conversion function directly should make it simple.

@eblot
Copy link
Owner

eblot commented Jun 18, 2021

I think you are very right.
This code has not been maintained for over 10 years, and is not part of the CI/CD.

Patches are very welcome - there are even better if you can include some tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Confirmed/reproduced bug
Projects
None yet
Development

No branches or pull requests

2 participants