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

#16475: Support object instancing and recursion in marshal

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 years, 2 months ago by sweskman
Modified:
6 years, 2 months ago
Reviewers:
martin, storchaka
CC:
loewis, gregory.p.smith, amaury.forgeotdarc, mark.dickinson, AntoinePitrou, krisvale, christian.heimes, Benjamin Peterson, ezio.melotti, devnull_psf.upfronthosting.co.za, storchaka
Visibility:
Public.

Patch Set 1 #

Patch Set 2 #

Total comments: 34

Patch Set 3 #

Total comments: 21

Patch Set 4 #

Total comments: 2

Patch Set 5 #

Unified diffs Side-by-side diffs Delta from patch set Stats Patch
Doc/library/marshal.rst View 1 2 3 4 2 chunks +6 lines, -3 lines 0 comments Download
Include/marshal.h View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
Lib/test/test_marshal.py View 1 2 3 4 3 chunks +125 lines, -2 lines 0 comments Download
Python/marshal.c View 1 2 3 4 48 chunks +245 lines, -27 lines 0 comments Download

Messages

Total messages: 11
loewis
I think at least the flag processing needs to be fixed before the patch can ...
6 years, 2 months ago #1
storchaka_gmail.com
I agree with all Martin's comments. http://bugs.python.org/review/16475/diff/6583/Python/marshal.c File Python/marshal.c (right): http://bugs.python.org/review/16475/diff/6583/Python/marshal.c#newcode5 Python/marshal.c:5: it would have ...
6 years, 2 months ago #2
kristjan_ccpgames.com
Thanks for your comments. I have dropped the "LREF" support and made various minor fixes ...
6 years, 2 months ago #3
storchaka_gmail.com
http://bugs.python.org/review/16475/diff/6583/Python/marshal.c File Python/marshal.c (right): http://bugs.python.org/review/16475/diff/6583/Python/marshal.c#newcode146 Python/marshal.c:146: w_type(char t, char flag, WFILE *p) On 2012/11/19 18:05:21, ...
6 years, 2 months ago #4
kristjan_ccpgames.com
http://bugs.python.org/review/16475/diff/6583/Python/marshal.c File Python/marshal.c (right): http://bugs.python.org/review/16475/diff/6583/Python/marshal.c#newcode146 Python/marshal.c:146: w_type(char t, char flag, WFILE *p) On 2012/11/19 19:41:34, ...
6 years, 2 months ago #5
storchaka_gmail.com
http://bugs.python.org/review/16475/diff/6631/Objects/setobject.c File Objects/setobject.c (right): http://bugs.python.org/review/16475/diff/6631/Objects/setobject.c#newcode2358 Objects/setobject.c:2358: PyErr_BadInternalCall(); This is unrelated changes. http://bugs.python.org/review/16475/diff/6631/Python/marshal.c File Python/marshal.c (right): ...
6 years, 2 months ago #6
storchaka_gmail.com
http://bugs.python.org/review/16475/diff/6631/Doc/library/marshal.rst File Doc/library/marshal.rst (right): http://bugs.python.org/review/16475/diff/6631/Doc/library/marshal.rst#newcode46 Doc/library/marshal.rst:46: For format *version* lower than 3, Recursive lists, sets ...
6 years, 2 months ago #7
kristjan_ccpgames.com
http://bugs.python.org/review/16475/diff/6631/Doc/library/marshal.rst File Doc/library/marshal.rst (right): http://bugs.python.org/review/16475/diff/6631/Doc/library/marshal.rst#newcode46 Doc/library/marshal.rst:46: For format *version* lower than 3, Recursive lists, sets ...
6 years, 2 months ago #8
storchaka_gmail.com
http://bugs.python.org/review/16475/diff/6631/Python/marshal.c File Python/marshal.c (right): http://bugs.python.org/review/16475/diff/6631/Python/marshal.c#newcode750 Python/marshal.c:750: r_ref_reserve(PyObject *o, Py_ssize_t *idx, int flag, RFILE *p) On ...
6 years, 2 months ago #9
storchaka_gmail.com
6 years, 2 months ago #10
kristjan_ccpgames.com
6 years, 2 months ago #11
http://bugs.python.org/review/16475/diff/6631/Python/marshal.c
File Python/marshal.c (right):

http://bugs.python.org/review/16475/diff/6631/Python/marshal.c#newcode750
Python/marshal.c:750: r_ref_reserve(PyObject *o, Py_ssize_t *idx, int flag,
RFILE *p)
On 2012/11/20 16:28:37, storchaka wrote:
> On 2012/11/20 14:44:01, krisvale wrote:
> > On 2012/11/20 14:06:28, storchaka wrote:
> > > r_ref_reserve() can just return idx (-1 means error).
> > I had it like that at first.  I changed it to this signature because it
> matches
> > better r_ref_insert and it simplifies error handling elsewhere (see line
> 1147). 
> > In other words:  This is on purpose.
> 
> There are to places where this function used. On second of them return value
> ignored and you test idx.
Yes.  Like I said, this is on purpose, to simplify error handling.  You can
either pass in a PyObject* and have it freed for you and check for NULL, or look
at the idx (when you don't already have a PyObject to pass in.

http://bugs.python.org/review/16475/diff/6631/Python/marshal.c#newcode752
Python/marshal.c:752: if (flag & FLAG_REF) {
On 2012/11/20 16:28:37, storchaka wrote:
> On 2012/11/20 14:44:01, krisvale wrote:
> > On 2012/11/20 14:06:28, storchaka wrote:
> > > if (flag) {
> > > 
> > > flag can be only 0 or FLAG_REF.
> > Currently, yes.  There is no harm in planning for the future.
> 
> Don't overplan. You have only one free bit, '0'|'N'|'T'|... = 0x7F.
> 
> Actually you can move this check outside r_ref*() functions. This will
simplify
> signatures.
Ok, I'll change the test to non-zero with a comment.
I like the test within the function where it is.

http://bugs.python.org/review/16475/diff/6632/Python/marshal.c
File Python/marshal.c (right):

http://bugs.python.org/review/16475/diff/6632/Python/marshal.c#newcode221
Python/marshal.c:221: PyErr_SetString(PyExc_ValueError, "too many objects");
On 2012/11/20 16:28:37, storchaka wrote:
> I think silent pass is good here. This is not error, this only means that you
> can't create new references after 0x7fffffff. However you can write and read a
> data even with more count of objects.

Well, it means that we are breaking the promise of protocol 3 and not sharing
references.  I think at such time in the future when we reach 2G objects in a
marshal stream, it would be good to get the error and re-introduce the LREF
opcode :)
I can demote this to a warning, if you like.
Sign in to reply to this message.

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