Thomas> I have a couple of questions regarding this patch, if you don't
Thomas> mind....
Thomas,
I don't mind at all. I will remind folks that I am more the messenger than
the messiah on this one, however. Greg Ewing (greg@cosc.canterbury.ac.nz)
is the author of the change, so will be much better equipped than me to
reply to your comments. I'm more a fan of the construct who happened to be
in the right place at the right time. All I did was update Greg's patch to
work with 1.5.2+ (which became 1.6alpha, which became 2.0beta).
Thomas> For one, the Grammar for list comprehensions:
Thomas> '[' [testlist [list_iter]] ']'
Thomas> Should that really be 'testlist'?
Thomas> [1,2,3,4,5]
Thomas> is a list of 5 elements, but
Thomas> [1,2,3,4,5 if 1]
Thomas> is a list of one element, which is a 5-element tuple.
I'll take a look at it. I'm not much of a parser person and my reading of
the grammar is hampered by the fact that Grammar/Grammar and the
grammar in the language reference manual no longer mesh very well. (Is that
something that can be remedied?) I notice that in the comment at the top of
com_list_comprehension Greg said:
atom: '[' test list_iter ']'
which suggests that you are onto something and that the 'testlist' variant
is either a typo or a mistake that wasn't corrected in a later version of
Greg's thinking ...
Thomas> I'd say the Grammar would be more like this:
Thomas> atom: '(' [testlist] ')' | '[' [listmaker] ']' | '{' [dictmaker] '}' | '' testlist '' | NAME | NUMBER | STRING+
Thomas> listmaker: test ( list_iter | (',' test)* [','])
Thomas> list_iter: list_for | list_if
Thomas> list_for: 'for' exprlist 'in' testlist [list_iter]
Thomas> list_if: 'if' test [list_iter]
Thomas> And by coincidence this is also fits very nicely with what the
Thomas> 'range-literal' patch does ;-)
If we're going to have both we should probably have them work together, I
agree.
Thomas> Another issue is the code- and logic-duplication of the patch to
Thomas> compile.c: it looks like a large part was just copied from
Thomas> com_for_node, and the block-stuff commented out. 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
Thomas> easier to split the com_for_node into a function that compiles
Thomas> the header, and one that compiles the suite, and make
Thomas> com_list_for compile its own 'suite'. I'm hoping that's
Thomas> possible, because it would keep the code a lot cleaner, and it
Thomas> would make it easier for me to implement 'for i in x;j in y' and
Thomas> have it work for list comprehensions too ;-)
Come up with an alternate patch that does what you want. I think very few
people have actually looked at Greg's patch closely and that his original
patch was more a proof of concept than a ready-for-primetime chunk of code.
Thomas> Also, the patch creates a list, stores it locally, and calls
Thomas> append() to it inside the inner loop. 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 ? The
Thomas> for loops counts the number of loops anyway ;) and it would get
Thomas> rid of the fairly ugly tmpname thing altogether, I
Thomas> think. However, the logic of when to ROT_THREE and ROT_TWO and
Thomas> what not might run away from under us ;-P (But I'm facing that
Thomas> anyway, with the parallel-forloop)
Dunno.
Thomas> Alternatively, it would be nice to store the lists' append() in
Thomas> a local vrbl, to reduce the number of lookups ;-P Oh, and
Thomas> lastly, the patch is just screaming for a re-indent all over,
Thomas> and of some wel-placed comments... some of it was quite
Thomas> difficult to follow, compared to the rest of the code.
Yes, it does need to be reindented. Greg, can you give us some feedback on
Thomas's comments when you get a chance?
--
Skip Montanaro, skip@mojam.com, http://www.mojam.com/, http://www.musi-cal.com/
"To get what you want you must commit yourself for sometime" - fortune cookie
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