chromatic <[EMAIL PROTECTED]> wrote:
On Sunday 22 April 2007 17:38, Matt Diephouse wrote:
> The attached patch completely reworks Parrot_process_args. The changes
> are extensive and I think they make the code much clearer. Rather than
> just check it in, I thought I'd try to get feedback here to make sure
> that it is clearer to everyone else and not just to myself.
It's a lot clearer.
Good.
> This patch also fixes a few bugs:
> #40490: Flat/Slurpy Named Parameter Passing Errors
Yay!
I thought you might like that. :)
> A couple todo'd error condition tests
>
> I'm sure there is more that can be done to clean things up, but this
> is at least a start. I've already spent 15+ hours on this patch, so
> I'm ready to check things in and leave further improvements for
> another time.
I only noticed one thing (besides large swaths of removed code, which is
always nice). This code occurs multiple times:
+ /* if the :flat arg is empty, just go to the next arg */
+ if (!st->src.slurp_n) {
+ st->src.mode &= ~CALL_STATE_FLATTEN;
+ st->src.i++;
+ }
It's three lines; is it worth extracting somehow?
It could definitely be placed inside start_flatten(), but that would
make the code a little misleading, I think. I'm not sure it's worth
placing it in a function of its own; the transparency may be worth
something in this case. Having said that, I think this section of the
code could be cleaned up more with further refactoring down the road.
There's also a large ~20 line section of code that is repeated in this
patch that I'll move to a function before I commit.
--
Matt Diephouse
http://matt.diephouse.com
