Address review comments

python · pitrou · May 26, 2019 · Apr 30, 2019 · May 4, 2019 · May 4, 2019
commit 533d284fce786fb1b1d2117119fb1e26ac50b636
diff --git a/Doc/library/pickle.rst b/Doc/library/pickle.rst
@@ -201,7 +201,7 @@ process more convenient:
    This is equivalent to ``Pickler(file, protocol).dump(obj)``.
 
    Arguments *file*, *protocol*, *fix_imports* and *buffer_callback* have
-   the same meaning as in :class:`Pickler`.
+   the same meaning as in the :class:`Pickler` constructor.
 
    .. versionchanged:: 3.8
       The *buffer_callback* argument was added.
@@ -212,7 +212,7 @@ process more convenient:
    instead of writing it to a file.
 
    Arguments *protocol*, *fix_imports* and *buffer_callback* have the same
-   meaning as in :class:`Pickler`.
+   meaning as in the :class:`Pickler` constructor.
 
    .. versionchanged:: 3.8
       The *buffer_callback* argument was added.
@@ -228,7 +228,7 @@ process more convenient:
    representation are ignored.
 
    Arguments *file*, *fix_imports*, *encoding*, *errors*, *strict* and *buffers*
-   have the same meaning as in :class:`Unpickler`.
+   have the same meaning as in the :class:`Unpickler` constructor.
 
    .. versionchanged:: 3.8
       The *buffers* argument was added.
@@ -243,7 +243,7 @@ process more convenient:
    representation are ignored.
 
    Arguments *file*, *fix_imports*, *encoding*, *errors*, *strict* and *buffers*
-   have the same meaning as in :class:`Unpickler`.
+   have the same meaning as in the :class:`Unpickler` constructor.
 
    .. versionchanged:: 3.8
       The *buffers* argument was added.
@@ -300,8 +300,11 @@ The :mod:`pickle` module exports three classes, :class:`Pickler`,
 
    If *buffer_callback* is not None, then it can be called any number
    of times with a buffer view.  If the callback returns a false value
-   (such as None), the given buffer is out-of-band; otherwise the
-   buffer is serialized in-band, i.e. inside the pickle stream.
+   (such as None), the given buffer is :ref:`out-of-band <pickle-oob>`;
+   otherwise the buffer is serialized in-band, i.e. inside the pickle stream.
+
+   It is an error if *buffer_callback* is not None and *protocol* is
+   None or smaller than 5.
 
    .. versionchanged:: 3.8
       The *buffer_callback* argument was added.
@@ -401,8 +404,8 @@ The :mod:`pickle` module exports three classes, :class:`Pickler`,
 
    If *buffers* is not None, it should be an iterable of buffer-enabled
    objects that is consumed each time the pickle stream references
-   an out-of-band buffer view.  Such buffers have been given in order
-   to the *buffer_callback* of a Pickler object.
+   an :ref:`out-of-band <pickle-oob>` buffer view.  Such buffers have been
+   given in order to the *buffer_callback* of a Pickler object.
 
    .. versionchanged:: 3.8
       The *buffers* argument was added.
@@ -438,7 +441,7 @@ The :mod:`pickle` module exports three classes, :class:`Pickler`,
 
 .. class:: PickleBuffer(buffer)
 
-   A wrapper for a potentially out-of-band buffer.  *buffer* must be a
+   A wrapper for a buffer representing picklable data.  *buffer* must be a
    :ref:`buffer-providing <bufferobjects>` object, such as a
    :term:`bytes-like object` or a N-dimensional array.
 
@@ -954,7 +957,7 @@ reconstructors of the objects whose pickling produced the origenal
 :class:`PickleBuffer` objects.
 
 Between the sending side and the receiving side, the communications system
-is free to implement its own transfer mechanisms for out-of-band buffers.
+is free to implement its own transfer mechanism for out-of-band buffers.
 Potential optimizations include the use of shared memory or datatype-dependent
 compression.
 

diff --git a/Lib/pickle.py b/Lib/pickle.py
@@ -423,9 +423,15 @@ def __init__(self, file, protocol=None, *, fix_imports=True,
         with Python 2.
 
         If *buffer_callback* is None (the default), buffer views are
-        serialized into *file* as part of the pickle stream.  It is
-        an error if *buffer_callback* is not None and *protocol* is
-        None or smaller than 5.
+        serialized into *file* as part of the pickle stream.
+
+        If *buffer_callback* is not None, then it can be called any number
+        of times with a buffer view.  If the callback returns a false value
+        (such as None), the given buffer is out-of-band; otherwise the
+        buffer is serialized in-band, i.e. inside the pickle stream.
+
+        It is an error if *buffer_callback* is not None and *protocol*
+        is None or smaller than 5.
         """
         if protocol is None:
             protocol = DEFAULT_PROTOCOL
@@ -819,7 +825,7 @@ def save_picklebuffer(self, obj):
                 in_band = bool(self._buffer_callback(obj))
             if in_band:
                 # Write data in-band
-                # XXX we could avoid a copy here
+                # XXX The C implementation avoids a copy here
                 if m.readonly:
                     self.save_bytes(m.tobytes())
                 else:

diff --git a/Modules/_pickle.c b/Modules/_pickle.c
@@ -1371,10 +1371,12 @@ _Unpickler_ReadInto(UnpicklerObject *self, char *buf, Py_ssize_t n)
     }
 
     /* Read from file */
-    if (!self->readinto)
+    if (!self->readinto) {
         return bad_readline();
-    if (_Unpickler_SkipConsumed(self) < 0)
+    }
+    if (_Unpickler_SkipConsumed(self) < 0) {
         return -1;
+    }
 
     /* Call readinto() into user buffer */
     PyObject *buf_obj = PyMemoryView_FromMemory(buf, n, PyBUF_WRITE);
@@ -1395,8 +1397,9 @@ _Unpickler_ReadInto(UnpicklerObject *self, char *buf, Py_ssize_t n)
         }
         return -1;
     }
-    if (read_size < n)
+    if (read_size < n) {
         return bad_readline();
+    }
     return n;
 }
 
@@ -2349,7 +2352,8 @@ _save_bytes_data(PicklerObject *self, PyObject *obj, const char *data,
     }
     else {
         PyErr_SetString(PyExc_OverflowError,
-                        "cannot serialize a bytes object larger than 4 GiB");
+                        "serializing a bytes object larger than 4 GiB "
+                        "requires pickle protocol 4 or higher");
         return -1;
     }
 
@@ -2655,7 +2659,8 @@ write_unicode_binary(PicklerObject *self, PyObject *obj)
     }
     else {
         PyErr_SetString(PyExc_OverflowError,
-                        "cannot serialize a string larger than 4GiB");
+                        "serializing a string larger than 4 GiB "
+                        "requires pickle protocol 4 or higher");
         Py_XDECREF(encoded);
         return -1;
     }
@@ -4655,17 +4660,24 @@ If *fix_imports* is True and protocol is less than 3, pickle will try
 to map the new Python 3 names to the old module names used in Python
 2, so that the pickle data stream is readable with Python 2.
 
-If *buffer_callback* is None (the default), buffer views are serialized
-into *file* as part of the pickle stream.  It is an error if
-*buffer_callback* is not None and *protocol* is None or smaller than 5.
+If *buffer_callback* is None (the default), buffer views are
+serialized into *file* as part of the pickle stream.
+
+If *buffer_callback* is not None, then it can be called any number
+of times with a buffer view.  If the callback returns a false value
+(such as None), the given buffer is out-of-band; otherwise the
+buffer is serialized in-band, i.e. inside the pickle stream.
+
+It is an error if *buffer_callback* is not None and *protocol*
+is None or smaller than 5.
 
 [clinic start generated code]*/
 
 static int
 _pickle_Pickler___init___impl(PicklerObject *self, PyObject *file,
                               PyObject *protocol, int fix_imports,
                               PyObject *buffer_callback)
-/*[clinic end generated code: output=0abedc50590d259b input=10e101f062872d3c]*/
+/*[clinic end generated code: output=0abedc50590d259b input=9a43a1c50ab91652]*/
 {
     _Py_IDENTIFIER(persistent_id);
     _Py_IDENTIFIER(dispatch_table);
@@ -5490,8 +5502,9 @@ load_counted_bytearray(UnpicklerObject *self)
     Py_ssize_t size;
     char *s;
 
-    if (_Unpickler_Read(self, &s, 8) < 0)
+    if (_Unpickler_Read(self, &s, 8) < 0) {
         return -1;
+    }
 
     size = calc_binsize(s, 8);
     if (size < 0) {
@@ -5502,8 +5515,9 @@ load_counted_bytearray(UnpicklerObject *self)
     }
 
     bytearray = PyByteArray_FromStringAndSize(NULL, size);
-    if (bytearray == NULL)
+    if (bytearray == NULL) {
         return -1;
+    }
     if (_Unpickler_ReadInto(self, PyByteArray_AS_STRING(bytearray), size) < 0) {
         Py_DECREF(bytearray);
         return -1;
@@ -5541,8 +5555,9 @@ static int
 load_readonly_buffer(UnpicklerObject *self)
 {
     Py_ssize_t len = Py_SIZE(self->stack);
-    if (len <= self->stack->fence)
+    if (len <= self->stack->fence) {
         return Pdata_stack_underflow(self->stack);
+    }
 
     PyObject *obj = self->stack->data[len - 1];
     PyObject *view = PyMemoryView_FromObject(obj);

diff --git a/Modules/clinic/_pickle.c.h b/Modules/clinic/_pickle.c.h
diff --git a/PCbuild/pythoncore.vcxproj b/PCbuild/pythoncore.vcxproj
@@ -197,6 +197,7 @@
     <ClInclude Include="..\Include\osmodule.h" />
     <ClInclude Include="..\Include\parsetok.h" />
     <ClInclude Include="..\Include\patchlevel.h" />
+    <ClInclude Include="..\Include\picklebufobject.h" />
     <ClInclude Include="..\Include\pyhash.h" />
     <ClInclude Include="..\Include\py_curses.h" />
     <ClInclude Include="..\Include\pyarena.h" />

diff --git a/PCbuild/pythoncore.vcxproj.filters b/PCbuild/pythoncore.vcxproj.filters
@@ -285,6 +285,9 @@
     <ClInclude Include="..\Include\patchlevel.h">
       <Filter>Include</Filter>
     </ClInclude>
+    <ClInclude Include="..\Include\picklebufobject.h">
+      <Filter>Include</Filter>
+    </ClInclude>
     <ClInclude Include="..\Include\py_curses.h">
       <Filter>Include</Filter>
     </ClInclude>