On Sat, Jan 14, 2012 at 1:17 AM, Georg Brandl <g.bra...@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 | ncogh...@gmail.com | Brisbane, Australia _______________________________________________ Python-Dev mailing list Python-Dev@python.org http://mail.python.org/mailman/listinfo/python-dev Unsubscribe: http://mail.python.org/mailman/options/python-dev/archive%40mail-archive.com