This is an archive of the discontinued LLVM Phabricator instance.

python compat - iterator protocol
ClosedPublic

Authored by serge-sans-paille on Jan 3 2019, 3:19 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

michaelplatings requested changes to this revision.Jan 3 2019, 5:11 AM
michaelplatings added inline comments.
utils/gdb-scripts/prettyprinters.py
73 ↗(On Diff #180013)

I think this needs an associated

if sys.version_info.major == 2:
    next == __next__
172 ↗(On Diff #180013)

I think this needs an associated

if sys.version_info.major == 2:
    next == __next__
This revision now requires changes to proceed.Jan 3 2019, 5:11 AM
serge-sans-paille marked 2 inline comments as done.
michaelplatings requested changes to this revision.Jan 3 2019, 6:28 AM
michaelplatings added inline comments.
bindings/python/llvm/core.py
403 ↗(On Diff #180048)

== should be =

utils/gdb-scripts/prettyprinters.py
84 ↗(On Diff #180048)

Should be next = __next__
Or don't make this change at all as the code should already work with Python 3.

90 ↗(On Diff #180048)

This line is just wrong in the original. It creates a variable __next__ that is set to the built in next function. It serves no use and should be deleted.

175 ↗(On Diff #180048)

I see at line 183:
__next__ = next
which would be broken by this change. I suggest not making this change as the code should already work for Python 3.

This revision now requires changes to proceed.Jan 3 2019, 6:28 AM
serge-sans-paille marked 7 inline comments as done.
serge-sans-paille added inline comments.
utils/gdb-scripts/prettyprinters.py
84 ↗(On Diff #180048)

I made the move to make it clear that python3 should be the default standard, and the ugly boilerplate is the python2 compatibility.

90 ↗(On Diff #180048)

Correct! I've been in auto-fix mode for some of these commits, thanks *a lot* for spotting all these mistakes.

175 ↗(On Diff #180048)

For the reasons given above, I still made the change, and fixed the behavior below. I also checked the other next implementation.

michaelplatings requested changes to this revision.Jan 3 2019, 7:00 AM
michaelplatings added inline comments.
utils/gdb-scripts/prettyprinters.py
185 ↗(On Diff #180065)

makor -> major

This revision now requires changes to proceed.Jan 3 2019, 7:00 AM
serge-sans-paille marked an inline comment as done.
michaelplatings requested changes to this revision.Jan 3 2019, 7:24 AM
michaelplatings added inline comments.
bindings/python/llvm/core.py
248 ↗(On Diff #180069)

I had a poke around this file and found that self.function.next refers to the next property in the Function class. I don't think the Function class should be changed as it isn't intended to be an iterator, and therefore the change to this line should be reverted.

319 ↗(On Diff #180069)

Likewise, self.bb.next is a property of BasicBlock.
The change to this line should be reverted.

399 ↗(On Diff #180069)

Likewise, self.inst.next is a property of Instruction.
The change to this line should be reverted.

bindings/python/llvm/tests/test_core.py
101 ↗(On Diff #180069)

I think this refers to the next property of the Function class in core.py. The change on this line should be reverted.

This revision now requires changes to proceed.Jan 3 2019, 7:24 AM
serge-sans-paille marked 3 inline comments as done.
This revision is now accepted and ready to land.Jan 3 2019, 7:44 AM
This revision was automatically updated to reflect the committed changes.