Hi!

Thanks for reading through!

> 1.  There appear to be some spurious whitespace insertions in this
>     version of the patch.

Oh, that's probably my editor, I'll fix that.

> 2.  The terms "lamba" and "anonymous function" are being used
>     interchangeably.  If we're going to introduce new terminology, it
>     would be good to pick one name and use it consistently.  I don't
>     have a preference for which one is ultimately chosen.

Well, create_function uses an already-existing EG(lambda_count) and
names the function __lambda_$counter so I thought I'd use
CG(compiled_lambda_count) and name them __compiled_lambda_... But since
anonymous functions aren't REAL lambdas, I named them anonymous
elsewhere. But you're right, introducing duplicate terminology is a bad
idea, I'll change everything to lambda for consistency, even though it's
technically not 100% correct.

>     The term "lexical" could also be considered a competing term as
>     its used in part of the patch.

'lexical' is only used for the variables that are passed into the
closure, not for the closure itself.

> 3.  The "is_anonymous" flags could be zend_bool values instead of bare
>     integers, although that breaks the precedent started by some
>     related flags (such as "is_method").

You're right, zend_bool is a better idea. Since PHP 5.3 is going to
break binary compability anyway, would it do any harm changing the
types of the existing flags, too?

> 4.  This part of the zend_vm_def.h diff looks wrong (a stray "f"):
> 
>         -/*
>         +f/*

WTF? I thought I had already fixed that. Hmm, obviously I hadn't...

> Looks great overall!

Thanks!

Merry Christmas,
Christian

-- 
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

Reply via email to