[Python-Dev] list comprehensions again...

[Skip Montanaro]

    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