Thomas> '[' [testlist [list_iter]] ']'
Thomas> Should that really be 'testlist'?
I can't remember my exact thinking at the time, but probably
I decided that, for consistency with the rest of the language,
the most general form of expression possible, without making
the grammar ambiguous, should be allowed. Personally, I don't
mind very much either way, but if it were changed I expect
you'd get complaints that things like
[x,y for x in (1,2,3) for y in (4,5,6)]
didn't do what the programmer expected.
Thomas> And by coincidence this is also fits very nicely with what the
Thomas> 'range-literal' patch does ;-)
I'm not familiar with that one, so I can't comment.
Thomas> it looks like a large part was just copied from
Thomas> com_for_node, and the block-stuff commented out.
As Skip said, it was a proof-of-concept, and I wanted to get it
working with as few changes to existing code as possible, both
because I wasn't sure I understood it well enough to mess with
it, and to minimise the chance of conflict with other patches.
Thomas> Why is it
Thomas> commented out, by the way ? Don't exceptions or something need
Thomas> the block setup ? If the block-setup was kept in, it might be
The block setup is used to handle breaking out of the for loop.
Since it's impossible to put a 'break' inside a list constructor,
the block setup is not needed -- or at least that's what I decided
at the time. I don't *think* that leaving it out will upset the
exception mechanism, but I'm not absolutely sure. Can anyone
who knows more about how it works comment?
In any case, I agree that the two for-loop compiling procedures could
and should be refactored somehow.
Thomas> Isn't it possible for the
Thomas> patch to build the list on the stack, pushing individual
Thomas> elements on the stack and calling BUILD_LIST at the end ?
Unfortunately, no. The compiler needs to know the maximum amount
of stack used by the procedure, and in the case of a list
comprehension, the list size is not known until run time.
However, things could still be improved greatly. My recommendation
would be to define a new opcode such as
LIST_APPEND <n>
which would take an item off the top of the stack, reach down
<n> stack items and append it to the list found there. That
would get rid of the temporary variable as well.
Thomas> lastly, the patch is just screaming for a re-indent all over,
I sort of hoped that, with it becoming an official feature, someone
would go over the code manually afterwards, rather than just applying
the patch and leaving it at that. If it's deemed that "someone"
should be me, let me know and I'll see what I can do.
Greg Ewing, Computer Science Dept, +--------------------------------------+
University of Canterbury, | A citizen of NewZealandCorp, a |
Christchurch, New Zealand | wholly-owned subsidiary of USA Inc. |
greg@cosc.canterbury.ac.nz +--------------------------------------+
RetroSearch is an open source project built by @garambo | Open a GitHub Issue
Search and Browse the WWW like it's 1997 | Search results from DuckDuckGo
HTML:
3.2
| Encoding:
UTF-8
| Version:
0.7.4