This is an archive of the discontinued LLVM Phabricator instance.

[LNT] Python 3 support: adapt to map returning an iterator
ClosedPublic

Authored by thopre on Sep 20 2019, 1:37 AM.

Details

Summary

Adapt calls to map() to the fact it returns an iterator in Python 3 when
necessary. This was produced by running futurize's stage2
lib2to3.fixes.fix_map, with manual clean up to remove unecessary list
wrapping and simplify the code.

Diff Detail

Repository
rL LLVM

Event Timeline

thopre created this revision.Sep 20 2019, 1:37 AM
lnt/external/stats/pstat.py
513 ↗(On Diff #220975)

max(map(lambda item: len(makestr(item)), items))?

lnt/external/stats/stats.py
1632 ↗(On Diff #220975)

See above.

lnt/lnttool/main.py
173 ↗(On Diff #220975)

I believe the change to this line is not necessary.

lnt/server/db/migrate.py
83 ↗(On Diff #220975)

The unpacking works fine without the change even with Python 3.

thopre updated this revision to Diff 221301.Sep 23 2019, 5:23 AM
thopre marked 7 inline comments as done.

Remove list wrapping when not necessary and simplify code whenever possible

thopre marked an inline comment as done.Sep 23 2019, 5:24 AM
thopre added inline comments.
lnt/external/stats/stats.py
1568–1571 ↗(On Diff #220975)

These do not seem to be used later in the function, how can I check if these are global variables?

lnt/server/db/migrate.py
83 ↗(On Diff #220975)

Indeed, https://docs.python.org/3/reference/simple_stmts.html#grammar-token-assignment-stmt says:

Assignment of an object to a target list, optionally enclosed in parentheses or square brackets, is recursively defined as follows.

  • If the target list is a single target with no trailing comma, optionally in parentheses, the object is assigned to that target.
  • Else: The object must be an iterable with the same number of items as there are targets in the target list, and the items are assigned, from left to right, to the corresponding targets.
lnt/testing/__init__.py
383 ↗(On Diff #221301)

Undoing useless change I made just before committing https://reviews.llvm.org/D65751

lnt/external/stats/stats.py
1569 ↗(On Diff #221301)

tmp is consumed twice immediately below, so the list application would still be appropriate if we aren't removing this block altogether.

1575 ↗(On Diff #221301)

Drive by:
alldata = N.array(list(itertools.chain.from_iterable(lists)))

1568–1571 ↗(On Diff #220975)

From https://docs.python.org/3/faq/programming.html#what-are-the-rules-for-local-and-global-variables-in-python:

If a variable is assigned a value anywhere within the function’s body, it’s assumed to be a local unless explicitly declared as global.

lnt/external/stats/stats.py
1575 ↗(On Diff #221301)

Hmm, since we need a list anyway, maybe:

for list in lists:
    alldata.extend(list)

I don't know enough about the way the implementation works to know that the chain version would be handled as well.

thopre retitled this revision from [LNT] Python 3 support: adapt to map returning a view to [LNT] Python 3 support: adapt to map returning an iterator.Sep 23 2019, 2:57 PM
thopre edited the summary of this revision. (Show Details)
thopre updated this revision to Diff 221525.Sep 24 2019, 6:05 AM
thopre marked 4 inline comments as done.

Remove unused variables defined from calling map rather than fixing map call

lnt/external/stats/stats.py
1569 ↗(On Diff #221301)

Yes but map accepts an iterable so if the only uses are below (as the name tmp would imply) that change should be fine. I grepped LNT around for vars and means and couldn't find use elsewhere and git history isn't very informative. I've thus decided to remove the entire block.

hubert.reinterpretcast marked an inline comment as done.

LGTM with comments on further changes that could be made.

lnt/external/stats/pstat.py
965 ↗(On Diff #221525)

Not the subject of this patch, but the cmp might be a problem.

1025 ↗(On Diff #221525)

Ditto.

lnt/external/stats/stats.py
1266 ↗(On Diff #221525)

Another opportunity to do clean-up in another patch. It seems this is some attempt to preallocate n (and it happens in a few other places), but I haven't found anything that indicates that this isn't just dead code.

1569 ↗(On Diff #221301)

I agree with the removal of the block. On the now-moot point (in case it comes up in the future), by "consume" I did mean destructively consume. That is:

tmp = map(str, [0, 1])
a = list(tmp)
b = list(tmp)  # Empty list!
This revision is now accepted and ready to land.Sep 24 2019, 8:48 PM
thopre marked 4 inline comments as done.Sep 25 2019, 12:55 AM
thopre added inline comments.
lnt/external/stats/pstat.py
965 ↗(On Diff #221525)

Good catch again! I'll grep to see if there's more. I'll update the cmp patch accordingly.

lnt/external/stats/stats.py
1569 ↗(On Diff #221301)

Ah right silly me, of course. Thanks for pointing this out.

This revision was automatically updated to reflect the committed changes.
thopre marked an inline comment as done.
Herald added a project: Restricted Project. · View Herald TranscriptSep 25 2019, 1:01 AM