On Sat, Jul 08, 2000 at 05:32:40AM -0500, Skip Montanaro wrote:
> Since Fredrik's voice is the only one I heard, and on the off-chance that
> the CNRI/BeOpen discussions and resulting 1.6 release push things out a bit
> (allowing a feature thaw) I'll oblige.
I have a couple of questions regarding this patch, if you don't mind. I
didn't appoint myself arbiter of new code, and I'm certainly not a kind of
spanish inquisition, but I really have to scratch my head when reading this
patch (and the resulting code.) I hope you don't take it personal ;P
Also, I'm not criticising the new feature, just wondering about the
implementation.
For one, the Grammar for list comprehensions:
'[' [testlist [list_iter]] ']'
Should that really be 'testlist' ? As far as I can see, it should be 'test'
rather than 'testlist'. This is kind of nit-picky, but the presence of the
list_iter part completely changes the meaning of the testlist in front of
it:
[1,2,3,4,5]
is a list of 5 elements, but
[1,2,3,4,5 if 1]
is a list of one element, which is a 5-element tuple. I'd say the Grammar
would be more like this:
atom: '(' [testlist] ')' | '[' [listmaker] ']' | '{' [dictmaker] '}' | '' testlist '' | NAME | NUMBER | STRING+
listmaker: test ( list_iter | (',' test)* [','])
list_iter: list_for | list_if
list_for: 'for' exprlist 'in' testlist [list_iter]
list_if: 'if' test [list_iter]
And by coincidence this is also fits very nicely with what the
'range-literal' patch does ;-)
Another issue is the code- and logic-duplication of the patch to compile.c:
it looks like a large part was just copied from com_for_node, and the
block-stuff commented out. Why is it commented out, by the way ? Don't
exceptions or something need the block setup ? If the block-setup was kept
in, it might be easier to split the com_for_node into a function that
compiles the header, and one that compiles the suite, and make com_list_for
compile its own 'suite'. I'm hoping that's possible, because it would keep
the code a lot cleaner, and it would make it easier for me to implement 'for
i in x;j in y' and have it work for list comprehensions too ;-)
Also, the patch creates a list, stores it locally, and calls append() to it
inside the inner loop. Isn't it possible for the patch to build the list on
the stack, pushing individual elements on the stack and calling BUILD_LIST
at the end ? The for loops counts the number of loops anyway ;) and it would
get rid of the fairly ugly tmpname thing altogether, I think. However, the
logic of when to ROT_THREE and ROT_TWO and what not might run away from
under us ;-P (But I'm facing that anyway, with the parallel-forloop)
Alternatively, it would be nice to store the lists' append() in a local
vrbl, to reduce the number of lookups ;-P Oh, and lastly, the patch is just
screaming for a re-indent all over, and of some wel-placed comments... some
of it was quite difficult to follow, compared to the rest of the code.
Just-showing-I-care-ly y'rs,
--
Thomas Wouters <thomas@xs4all.net>
Hi! I'm a .signature virus! copy me into your .signature file to help me spread!
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