[Python-3000] PEP 3107 - Function Annotations

[Neal Norwitz]

On 12/28/06, Tony Lownds <tony at pagedna.com> wrote:
>
> >  * With the modification of MAKE_FUNCTION to be a 32-bit value, this
> > means that EXTENDED_ARG is now used.  This means that the peephole
> > optimizer won't work for the outer function.  I think we should
> > correct this.
>
> Ok. Something like this should fix it in peephople.c. I can post a
> patch.
>
> @@ -535,8 +535,13 @@
>                                  break;
>                          case EXTENDED_ARG:
> -                               goto exitUnchanged;
> +                               if (codestr[i+3] != MAKE_FUNCTION)
> +                                       goto exitUnchanged;
> +                               /* don't visit MAKE_FUNCTION as
> GETARG will be wrong */
> +                               i += 3;
> +                               break;
> +

I'm not sure if it's not that simple or not.  The problem is the jump
fix up code.  There are some assumptions that the code sizes are only
1 or 2.  It's possible that this is a very limited circumstance that
doesn't screw up any of the assumptions.  IIRC there would be problems
if a jump changed from 2 to 4 bytes (or vica versa).  Since the size
can't change for MAKE_FUNCTION, it may be ok in this limited case, but
I'm not sure without reviewing the code in detail.  (It probably is
ok, but I'm not sure.)

Hmm, what would happen if the nested function contained more than 256
args?  Might that mess up the annotation handling?

> >  * I don't understand why there was a new annotations (flag) parameter
> > added in symtable.  My guess was that this was to support lambdas
> > without annotations. If this is true, it would be nicer if we didn't
> > have to special case.  I think there was talk about requiring parens
> > around lambda params.  I don't recall the outcome though.
> >
>
> The flag is there to re-use the logic for visiting parameters, once
> for the
> annotations, once for the parameter names. Those need to be separate
> passes
> because the parameter names are added to the functions symbol table,
> annotations
> to the outer symbol table.

That's good to know.  If there isn't a comment about this in the code,
this info would be good to add.  I don't know if it's clear or not
from the resulting code.  Sometimes it's hard to tell what the code
looks like after applying the patch.  I typically only review the
patch itself, so I miss context.  Though it allows for faster reviews.
:-)

> > Tony, you cleaned up quite a few places in the AST, care to do more
> > cleanup?  I liked your refactorings.
>
> Sure, anything in particular?

Not without looking at the code for a while.  Anything you found
difficult, hard to understand, duplicated, etc.  These would be good
to clean up, if nothing else document more appropriately.  I suspect
quite a few comments are out of date.

> > I also don't love all the names in Grammar.  Guido had mentioned about
> > cleaning up the Grammar.  I don't know if he still wants to or if it
> > can be improved.  It would be nice once all the basic features are in
> > if we could make a cleanup pass for things like Grammar, obsolete
> > APIs, etc.
>
> The only name changes I made in Grammar were made to allow
> the same compiler code to process either lambda's arguments without
> annotations or def's arguments with annotations. fpdef became tfpdef
> and vfpdef, fplist became tfplist and vfplist. tname is a new
> nonterminal
> that holds the annotation itself, and vname is the corresponding version
> without an annotation. "tname" replaced NAME in many places.
>
> Maybe there is a better naming convention that "t" and "v" prefixes? Or
> perhaps "tname/vname" could be improved?

Yeah I think the names could be improved, but I don't have any ideas. :-(

n