[Python-Dev] cpython: Implement PEP 380 - 'yield from' (closes #11682)
Nick Coghlan
ncoghlan at gmail.com
Sat Jan 14 07:53:39 CET 2012
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
More information about the Python-Dev
mailing list