[Python-Dev] cpython: Implement PEP 380 - 'yield from' (closes #11682)

[Nick Coghlan]

On Sat, Jan 14, 2012 at 1:17 AM, Georg Brandl <g.brandl at gmx.net> wrote:
> On 01/13/2012 12:43 PM, nick.coghlan wrote:
>> diff --git a/Doc/reference/expressions.rst b/Doc/reference/expressions.rst
>
> There should probably be a "versionadded" somewhere on this page.

Good catch, I added versionchanged notes to this page, simple_stmts
and the StopIteration entry in the library reference.

>>  PEP 3155: Qualified name for classes and functions
>>  ==================================================
>
> This looks like a spurious (and syntax-breaking) change.

Yeah, it was an error I introduced last time I merged from default. Fixed.

>> diff --git a/Grammar/Grammar b/Grammar/Grammar
>> -argument: test [comp_for] | test '=' test  # Really [keyword '='] test
>> +argument: (test) [comp_for] | test '=' test  # Really [keyword '='] test
>
> This looks like a change without effect?

Fixed.

It was a lingering after-effect of Greg's original patch (which also
modified the function call syntax to allow "yield from" expressions
with extra parens). I reverted the change to the function call syntax,
but forgot to ditch the added parens while doing so.

>> diff --git a/Include/genobject.h b/Include/genobject.h
>>
>> -     /* List of weak reference. */
>> -     PyObject *gi_weakreflist;
>> +        /* List of weak reference. */
>> +        PyObject *gi_weakreflist;
>>  } PyGenObject;
>
> While these change tabs into spaces, it should be 4 spaces, not 8.

Fixed.

>> +PyAPI_FUNC(int) PyGen_FetchStopIterationValue(PyObject **);
>
> Does this API need to be public? If yes, it needs to be documented.

Hmm, good point - that one needs a bit of thought, so I've put it on
the tracker: http://bugs.python.org/issue13783

(that issue also covers your comments regarding the docstring for this
function and whether or not we even need the StopIteration instance
creation API)

>> -#define CALL_FUNCTION        131     /* #args + (#kwargs<<8) */
>> -#define MAKE_FUNCTION        132     /* #defaults + #kwdefaults<<8 + #annotations<<16 */
>> -#define BUILD_SLICE  133     /* Number of items */
>> +#define CALL_FUNCTION   131     /* #args + (#kwargs<<8) */
>> +#define MAKE_FUNCTION   132     /* #defaults + #kwdefaults<<8 + #annotations<<16 */
>> +#define BUILD_SLICE     133     /* Number of items */
>
> Not sure putting these and all the other cosmetic changes into an already
> big patch is such a good idea...

I agree, but it's one of the challenges of a long-lived branch like
the PEP 380 one (I believe some of these cosmetic changes started life
in Greg's original patch and separating them out would have been quite
a pain). Anyone that wants to see the gory details of the branch
history can take a look at my bitbucket repo:

https://bitbucket.org/ncoghlan/cpython_sandbox/changesets/tip/branch%28%22pep380%22%29

>> diff --git a/Objects/abstract.c b/Objects/abstract.c
>> --- a/Objects/abstract.c
>> +++ b/Objects/abstract.c
>> @@ -2267,7 +2267,6 @@
>>
>>      func = PyObject_GetAttrString(o, name);
>>      if (func == NULL) {
>> -        PyErr_SetString(PyExc_AttributeError, name);
>>          return 0;
>>      }
>>
>> @@ -2311,7 +2310,6 @@
>>
>>      func = PyObject_GetAttrString(o, name);
>>      if (func == NULL) {
>> -        PyErr_SetString(PyExc_AttributeError, name);
>>          return 0;
>>      }
>>      va_start(va, format);
>
> These two changes also look suspiciously unrelated?

IIRC, I removed those lines while working on the patch because the
message they produce (just the attribute name) is worse than the one
produced by the call to PyObject_GetAttrString (which also includes
the type of the object being accessed). Leaving the original
exceptions alone helped me track down some failures I was getting at
the time.

I've now made the various CallMethod helper APIs in abstract.c (1
public, 3 private) consistently leave the GetAttr exception alone and
added an explicit C API note to NEWS.

(Vaguely related tangent: the new code added by the patch probably has
a few parts that could benefit from the new GetAttrId private API)

>> diff --git a/Objects/genobject.c b/Objects/genobject.c
>> +        } else {
>> +            PyObject *e = PyStopIteration_Create(result);
>> +            if (e != NULL) {
>> +                PyErr_SetObject(PyExc_StopIteration, e);
>> +                Py_DECREF(e);
>> +            }
>
> Wouldn't PyErr_SetObject(PyExc_StopIteration, value) suffice here
> anyway?

I think you're right - so noted in the tracker issue about the C API additions.

Thanks for the thorough review, a fresh set of eyes is very helpful :)

Cheers,
Nick.

-- 
Nick Coghlan   |   ncoghlan at gmail.com   |   Brisbane, Australia