Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(13400)

#18112: PEP 442 implementation

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 years, 4 months ago by pitrou
Modified:
6 years, 2 months ago
Reviewers:
benjamin
CC:
AntoinePitrou, haypo, christian.heimes, Benjamin Peterson, devnull_psf.upfronthosting.co.za, pconnell, isoschiz
Visibility:
Public.

Patch Set 1 #

Patch Set 2 #

Total comments: 21

Patch Set 3 #

Patch Set 4 #

Patch Set 5 #

Unified diffs Side-by-side diffs Delta from patch set Stats Patch
Doc/c-api/typeobj.rst View 1 2 3 4 2 chunks +45 lines, -0 lines 0 comments Download
Doc/includes/typestruct.h View 1 2 3 4 1 chunk +7 lines, -0 lines 0 comments Download
Doc/library/gc.rst View 1 2 3 4 2 chunks +10 lines, -17 lines 0 comments Download
Doc/library/weakref.rst View 1 2 3 4 1 chunk +6 lines, -15 lines 0 comments Download
Doc/reference/datamodel.rst View 1 2 3 4 1 chunk +4 lines, -6 lines 0 comments Download
Include/object.h View 1 2 3 4 3 chunks +10 lines, -0 lines 0 comments Download
Include/objimpl.h View 1 2 3 4 4 chunks +29 lines, -5 lines 0 comments Download
Lib/test/test_descr.py View 1 2 3 4 1 chunk +0 lines, -10 lines 0 comments Download
Lib/test/test_finalization.py View 1 2 3 4 1 chunk +513 lines, -0 lines 0 comments Download
Lib/test/test_gc.py View 1 2 3 4 6 chunks +12 lines, -6 lines 0 comments Download
Lib/test/test_generators.py View 1 2 3 4 2 chunks +53 lines, -0 lines 0 comments Download
Lib/test/test_io.py View 1 2 3 4 3 chunks +24 lines, -21 lines 0 comments Download
Lib/test/test_sys.py View 1 2 3 4 1 chunk +2 lines, -2 lines 0 comments Download
Modules/gcmodule.c View 1 2 3 4 24 chunks +107 lines, -49 lines 0 comments Download
Modules/_io/bufferedio.c View 1 2 3 4 17 chunks +63 lines, -11 lines 0 comments Download
Modules/_io/fileio.c View 1 2 3 4 7 chunks +19 lines, -5 lines 0 comments Download
Modules/_io/iobase.c View 1 2 3 4 8 chunks +52 lines, -40 lines 0 comments Download
Modules/_io/textio.c View 1 2 3 4 9 chunks +29 lines, -8 lines 0 comments Download
Modules/_testcapimodule.c View 1 2 3 4 2 chunks +80 lines, -0 lines 0 comments Download
Objects/genobject.c View 1 2 3 4 6 chunks +32 lines, -71 lines 0 comments Download
Objects/object.c View 1 2 3 4 3 chunks +68 lines, -2 lines 0 comments Download
Objects/typeobject.c View 1 2 3 4 9 chunks +47 lines, -50 lines 0 comments Download

Messages

Total messages: 2
Benjamin Peterson
I'm just publishing comments I had on an old version. Some of these might not ...
6 years, 2 months ago #1
AntoinePitrou
6 years, 2 months ago #2
http://bugs.python.org/review/18112/diff/8407/Include/object.h
File Include/object.h (right):

http://bugs.python.org/review/18112/diff/8407/Include/object.h#newcode657
Include/object.h:657: #define Py_TPFLAGS_HAVE_FINALIZE (1UL << 0)
On 2013/07/27 23:09:30, Benjamin Peterson wrote:
> Why not just put this at top of the flags list?

I thought I would leave the flags in chronological order of appearance, but I
don't mind putting it at top.

http://bugs.python.org/review/18112/diff/8407/Include/objimpl.h
File Include/objimpl.h (right):

http://bugs.python.org/review/18112/diff/8407/Include/objimpl.h#newcode272
Include/objimpl.h:272: #define _PyGCHead_SET_REFS(g, v) do { \
On 2013/07/27 23:09:30, Benjamin Peterson wrote:
> Why does only this macro use do.. while?

To avoid the trailing semicolon. But, you're right, it doesn't seem to be
necessary :-)

http://bugs.python.org/review/18112/diff/8407/Modules/_io/fileio.c
File Modules/_io/fileio.c (right):

http://bugs.python.org/review/18112/diff/8407/Modules/_io/fileio.c#newcode54
Modules/_io/fileio.c:54: char finalizing;
On 2013/07/27 23:09:30, Benjamin Peterson wrote:
> Can't this stay in the bitfield?

Not if we want to be lazy and use T_BOOL to make it readable as an attribute.

http://bugs.python.org/review/18112/diff/8407/Modules/_io/fileio.c#newcode1239
Modules/_io/fileio.c:1239: 0, /* tp_finalize */
On 2013/07/27 23:09:30, Benjamin Peterson wrote:
> Need to fix the alignment here, although I don't see what you added all these
0
> fields...

Perhaps I should add a comment explaining why?
The reason is that tp_finalize is overriden by PyType_Ready (it is inherited
from IOBase), but for that the field has to be allocated in the first place
(hence also the Py_TPFLAGS_HAVE_FINALIZE).

http://bugs.python.org/review/18112/diff/8407/Modules/_io/iobase.c
File Modules/_io/iobase.c (right):

http://bugs.python.org/review/18112/diff/8407/Modules/_io/iobase.c#newcode223
Modules/_io/iobase.c:223: if (PyObject_SetAttrString(self, "_finalizing",
Py_True))
On 2013/07/27 23:09:30, Benjamin Peterson wrote:
> A nice place to use _Py_identifier and PyObject_SetAttrId.

Will do :-)

http://bugs.python.org/review/18112/diff/8407/Modules/_testcapimodule.c
File Modules/_testcapimodule.c (right):

http://bugs.python.org/review/18112/diff/8407/Modules/_testcapimodule.c#newco...
Modules/_testcapimodule.c:2546: tp->tp_del = slot_tp_del;
On 2013/07/27 23:09:30, Benjamin Peterson wrote:
> Dangerous if tp->tp_del != NULL...

Yeah. It's only _testcapi, though.

http://bugs.python.org/review/18112/diff/8407/Modules/gcmodule.c
File Modules/gcmodule.c (right):

http://bugs.python.org/review/18112/diff/8407/Modules/gcmodule.c#newcode747
Modules/gcmodule.c:747: /* Handle uncollectable garbage (cycles with finalizers,
and stuff reachable
On 2013/07/27 23:09:30, Benjamin Peterson wrote:
> Also this comment.

Will do.

http://bugs.python.org/review/18112/diff/8407/Modules/gcmodule.c#newcode1722
Modules/gcmodule.c:1722: g->gc.gc_refs = 0;
On 2013/07/27 23:09:30, Benjamin Peterson wrote:
> Is this line useful anymore?

Yes, because _PyGCHead_SET_REFS() only switches one bit.
Sign in to reply to this message.

RSS Feeds Recent Issues | This issue
This is Rietveld 894c83f36cb7+