reversed() doesn't work on intbv

[xesscorp]

I declared an intbv and I could iterate through the bits in normal order. But if I used reversed() on the intbv to iterate through the bits in the opposite order, I got the same result and no error message.

In [1]: from myhdl import *

In [2]: v = intbv('01010001')

In [3]: for b in v:
   ...:     print(b)
   ...:
False
True
False
True
False
False
False
True

In [4]: for b in reversed(v):
   ...:     print(b)
   ...:
False
True
False
True
False
False
False
True

[josyb]

From the Python documentation:

Return a reverse iterator. seq must be an object which has a reversed() method or supports the sequence protocol (the len() method and the getitem() method with integer arguments starting at 0).

intbv has the len() and getitem() methods, but no reversed() method, so Python is doing the work for us.

I modified my local _intbv.py to show what's going on:

        else:
            i = int(key)
            bres = bool((self._val >> i) & 0x1)
            print('intbv getitem - index', i, bres)
            return bres

`>>> from myhdl import *

v = intbv('01010001')
for b in v:
... pass
...
('intbv getitem', 7, False)
('intbv getitem', 6, True)
('intbv getitem', 5, False)
('intbv getitem', 4, True)
('intbv getitem', 3, False)
('intbv getitem', 2, False)
('intbv getitem', 1, False)
('intbv getitem', 0, True)
for b in reversed(v):
... pass
...
('intbv getitem', 7, False)
('intbv getitem', 6, True)
('intbv getitem', 5, False)
('intbv getitem', 4, True)
('intbv getitem', 3, False)
('intbv getitem', 2, False)
('intbv getitem', 1, False)
('intbv getitem', 0, True)
`

BTW: IMHO the first sequence is the incorrect one ;) (as it starts with bit 7 and not bit 0)

[cfelton]

It looks like the intbv does not implement any iteration methods (__iter__ or __reversed__). At this point in time reversed is not support (i.e. a limitation) but it could be considered as an enhancement (?).

To make this clear to users, maybe an error should be raised if an intbvis iterated? Although iterations could be useful for modeling my guess is it would be some work to make it convertible.

[josyb]

intbv does have an iter() method:

    # iterator method
    def __iter__(self):
        if not self._nrbits:
            raise TypeError("Cannot iterate over unsized intbv")
        return iter([self[i] for i in range(self._nrbits-1, -1, -1)])

The complete description for reversed() from the Python documentation (I forgot the first line):

reversed(seq)
Return a reverse iterator. seq must be an object which has a reversed() method or supports the sequence protocol (the len() method and the getitem() method with integer arguments starting at 0).
New in version 2.4.
Changed in version 2.6: Added the possibility to write a custom reversed() method.

I added one more printout to the _intbv.py code, this shows what's going on underneath, and where the error lies:

from myhdl import *
v = intbv('01010001')
for b in v:
... pass
...
returning iterator over intbv
('intbv getitem - index', 7, False)
('intbv getitem - index', 6, True)
('intbv getitem - index', 5, False)
('intbv getitem - index', 4, True)
('intbv getitem - index', 3, False)
('intbv getitem - index', 2, False)
('intbv getitem - index', 1, False)
('intbv getitem - index', 0, True)

for b in reversed(v):
... pass
...
('intbv getitem - index', 7, False)
('intbv getitem - index', 6, True)
('intbv getitem - index', 5, False)
('intbv getitem - index', 4, True)
('intbv getitem - index', 3, False)
('intbv getitem - index', 2, False)
('intbv getitem - index', 1, False)
('intbv getitem - index', 0, True)

And as I suspected, the first iteration, which makes use of the supplied iter() method, is incorrect - according to Python rules that you start with element 0 first.

[cfelton]

@josyb thanks for the clarification, guess I should have looked at the source before commenting :)

It looks like the reversed order is intentional

[josyb]

@cfelton It may well have been intentional, but IMO it is wrong. And counter-intuitive.
With hindsight (always easy ...) reversing the indexes (from high to low in stead of the standard Python low to high) for the intbv type may be flawed. But yes, we will/can live with that.
If we keep the intentionally reversed iter() order, all we can (have to) do is to implement the reversed() method.

[cfelton]

@josyb I agree, this should be recorded as a known limitation and a future enhancement will be to implemented __reversed__.

As far as deciding which directions the iterations should occur that is a conversation that will need to take place with @jandecaluwe (and others?). Guess you can throw it out on gitter and see what the consensus is.

My intention of including the original commit/change was to show that it isn't a bug but a difference in opinion on the expected behavior. The later take more effort to resolve :( I couldn't extract from the comment why the order was changed.

Also, I assume that the iterations are not convertible? Is that correct?

I believe our informal process is to track enhancements on discourse.myhdl.org and bugs with github. My inclination is to add some documentation on intbv iterations (here or here ) and create a discourse topic for reversed and then close this issue (the issues and prs are a little out of control).

[josyb]

@cfelton Yes, let's continue on discourse.

[tokencolour]

Hello Devs, I did search on discourse but couldn't find the rest of the discussion there. Can you please direct me to it?

[josyb]

@tokencolour This discussion has been transferred to discourse yet.
@cfelton I vote to classify this as a bug and keep the discussion here.

[davekeeshan]

Restarting discussion here. I think a decision should be made on this issue can be closed and the PR assocated can be merged or denied.

The problem, I believe, is that the iterator delivers the vector from a sized intbv starting with the MSB down to the LSB. Whereas most people would naturally expect it from LSB to MSB.

The additional issue is that the reversed order was the same as a straight iter (though arguably that actually made it correct)

This is just my opinion on the topic, I respect that this may not match other peoples:

As a Hardware person I would expect it to arrive LSB to MSB and as a python person I would expect an array to start from 0.

This

        for i, v in enumerate(vector):
            assert v == b[i]

Should be the same as this:

        for i, v in enumerate(vector):
            assert vector[i] == b[i]

Which currently it is not.

There is a comment about if it was changed that it may break exisiting code. I feel very mixed about this, I respect that this may be being used in the field and a fix would break it. Bugs are fixed all the time, and this is a bug! However, I would actually be surprised if this is being used in the field right now if it's current operation is in reverse order, I imagine if someone tried to use it and found the current operation they would have rewritten it to loop on i and use the i index

As a reference, apart from the tests that explictly test it in the pytest suite, it does not apppear any where else in the myhdl tests.

Is there any recorded reason why it is being done the way it is being done?