From 75856fb8f1715f5d8ee4aa04d723f53bd31bcf27 Mon Sep 17 00:00:00 2001 From: thautwarm Date: Sun, 14 Oct 2018 04:41:33 +0800 Subject: [PATCH 01/16] bpo-34953: Implement `mmap.mmap.__repr__` --- Modules/mmapmodule.c | 52 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/Modules/mmapmodule.c b/Modules/mmapmodule.c index 86632358b7fc2d9..f53a0fc857116d5 100644 --- a/Modules/mmapmodule.c +++ b/Modules/mmapmodule.c @@ -682,6 +682,58 @@ mmap__exit__method(PyObject *self, PyObject *args) return _PyObject_CallMethodId(self, &PyId_close, NULL); } +static PyObject * +mmap__repr__method(PyObject *self) +{ + Py_ssize_t num_bytes = PY_SSIZE_T_MAX, remaining; + PyObject *repr; + mmap_object *m_obj; + m_obj = (mmap_object *) self; + const char *reprfmt; + const char *access_str; + + switch (m_obj->access) + { + case ACCESS_DEFAULT: + access_str = "ACCESS_DEFAULT"; + break; + + case ACCESS_READ: + access_str = "ACCESS_READ"; + break; + + case ACCESS_WRITE: + access_str = "ACCESS_WRITE"; + break; + case ACCESS_COPY: + access_str = "ACCESS_COPY"; + break; + default: + PyErr_SetString(PyExc_IOError, "Unknown access mode for mmap object."); + return NULL; + } + + if (m_obj -> data == NULL) + { + reprfmt = "<%s is_closed=True fileno=%d access=%s>"; + repr = PyUnicode_FromFormat(reprfmt, self->ob_type->tp_name, m_obj->fd, access_str); + } + else { + reprfmt = "<%s is_closed=False fileno=%d access=%s length=%s offset=%s entire_contents=%s>"; + PyObject *entire_contents; + entire_contents = PyBytes_FromStringAndSize(&m_obj->data, m_obj->size); + repr = PyUnicode_FromFormat(reprfmt, + self->ob_type->tp_name, + m_obj->fd, + access_str, + PyLong_FromSize_t(m_obj->size), + PyLong_FromSize_t(m_obj->pos), + entire_contents); + } + + return repr; +} + #ifdef MS_WINDOWS static PyObject * mmap__sizeof__method(mmap_object *self, void *unused) From 718a6b074bc685e9057c0d881103a42a598301fa Mon Sep 17 00:00:00 2001 From: thautwarm Date: Sun, 14 Oct 2018 17:19:21 +0800 Subject: [PATCH 02/16] done --- Modules/mmapmodule.c | 48 +++++++++++++++++++++++++++++++------------- 1 file changed, 34 insertions(+), 14 deletions(-) diff --git a/Modules/mmapmodule.c b/Modules/mmapmodule.c index f53a0fc857116d5..6db269cad14a4eb 100644 --- a/Modules/mmapmodule.c +++ b/Modules/mmapmodule.c @@ -685,13 +685,15 @@ mmap__exit__method(PyObject *self, PyObject *args) static PyObject * mmap__repr__method(PyObject *self) { - Py_ssize_t num_bytes = PY_SSIZE_T_MAX, remaining; PyObject *repr; mmap_object *m_obj; m_obj = (mmap_object *) self; + const char *reprfmt; const char *access_str; - + + Py_ssize_t size = m_obj->size; + switch (m_obj->access) { case ACCESS_DEFAULT: @@ -705,9 +707,11 @@ mmap__repr__method(PyObject *self) case ACCESS_WRITE: access_str = "ACCESS_WRITE"; break; + case ACCESS_COPY: access_str = "ACCESS_COPY"; break; + default: PyErr_SetString(PyExc_IOError, "Unknown access mode for mmap object."); return NULL; @@ -718,17 +722,32 @@ mmap__repr__method(PyObject *self) reprfmt = "<%s is_closed=True fileno=%d access=%s>"; repr = PyUnicode_FromFormat(reprfmt, self->ob_type->tp_name, m_obj->fd, access_str); } - else { - reprfmt = "<%s is_closed=False fileno=%d access=%s length=%s offset=%s entire_contents=%s>"; - PyObject *entire_contents; - entire_contents = PyBytes_FromStringAndSize(&m_obj->data, m_obj->size); - repr = PyUnicode_FromFormat(reprfmt, - self->ob_type->tp_name, - m_obj->fd, - access_str, - PyLong_FromSize_t(m_obj->size), - PyLong_FromSize_t(m_obj->pos), - entire_contents); + else + { + const char* tp_name; + int fd; + tp_name = self->ob_type->tp_name; + fd = m_obj->fd; + + if (size < 100) + { + reprfmt = "<%s is_closed=False fileno=%d access=%s length=%R offset=%R entire_contents=%R>"; + PyObject *entire_contents; + entire_contents = PyBytes_FromStringAndSize(&m_obj->data, size); + repr = PyUnicode_FromFormat(reprfmt, tp_name, fd, access_str, + PyLong_FromSize_t(m_obj->size), + PyLong_FromSize_t(m_obj->pos), + entire_contents); + } + else + { + reprfmt = "<%s is_closed=False fileno=%d access=%s length=%R offset=%R entire_contents=%R ... %R>"; + repr = PyUnicode_FromFormat(reprfmt, tp_name, fd, access_str, + PyLong_FromSize_t(m_obj->size), + PyLong_FromSize_t(m_obj->pos), + PyBytes_FromStringAndSize(&m_obj->data, 50), + PyBytes_FromStringAndSize(&m_obj->data + size - 50, 50)); + } } return repr; @@ -764,6 +783,7 @@ static struct PyMethodDef mmap_object_methods[] = { {"write_byte", (PyCFunction) mmap_write_byte_method, METH_VARARGS}, {"__enter__", (PyCFunction) mmap__enter__method, METH_NOARGS}, {"__exit__", (PyCFunction) mmap__exit__method, METH_VARARGS}, + {"__repr__", (PyCFunction) mmap__repr__method, METH_NOARGS}, #ifdef MS_WINDOWS {"__sizeof__", (PyCFunction) mmap__sizeof__method, METH_NOARGS}, #endif @@ -1043,7 +1063,7 @@ static PyTypeObject mmap_object_type = { 0, /* tp_getattr */ 0, /* tp_setattr */ 0, /* tp_reserved */ - 0, /* tp_repr */ + (reprfunc) mmap__repr__method, /* tp_repr */ 0, /* tp_as_number */ &mmap_as_sequence, /*tp_as_sequence*/ &mmap_as_mapping, /*tp_as_mapping*/ From c6900eee44f2d9ec8c4aacf44a5e8ac2e11411af Mon Sep 17 00:00:00 2001 From: thautwarm Date: Mon, 15 Oct 2018 14:29:03 +0800 Subject: [PATCH 03/16] follow PEP7 --- Modules/mmapmodule.c | 32 ++++++++++++++------------------ 1 file changed, 14 insertions(+), 18 deletions(-) diff --git a/Modules/mmapmodule.c b/Modules/mmapmodule.c index 6db269cad14a4eb..3a1db70795bad0e 100644 --- a/Modules/mmapmodule.c +++ b/Modules/mmapmodule.c @@ -694,16 +694,15 @@ mmap__repr__method(PyObject *self) Py_ssize_t size = m_obj->size; - switch (m_obj->access) - { - case ACCESS_DEFAULT: + switch (m_obj->access) { + case ACCESS_DEFAULT: access_str = "ACCESS_DEFAULT"; break; - + case ACCESS_READ: access_str = "ACCESS_READ"; break; - + case ACCESS_WRITE: access_str = "ACCESS_WRITE"; break; @@ -717,13 +716,11 @@ mmap__repr__method(PyObject *self) return NULL; } - if (m_obj -> data == NULL) - { + if (m_obj -> data == NULL) { reprfmt = "<%s is_closed=True fileno=%d access=%s>"; repr = PyUnicode_FromFormat(reprfmt, self->ob_type->tp_name, m_obj->fd, access_str); } - else - { + else { const char* tp_name; int fd; tp_name = self->ob_type->tp_name; @@ -734,21 +731,20 @@ mmap__repr__method(PyObject *self) reprfmt = "<%s is_closed=False fileno=%d access=%s length=%R offset=%R entire_contents=%R>"; PyObject *entire_contents; entire_contents = PyBytes_FromStringAndSize(&m_obj->data, size); - repr = PyUnicode_FromFormat(reprfmt, tp_name, fd, access_str, - PyLong_FromSize_t(m_obj->size), - PyLong_FromSize_t(m_obj->pos), + repr = PyUnicode_FromFormat(reprfmt, tp_name, fd, access_str, + PyLong_FromSize_t(m_obj->size), + PyLong_FromSize_t(m_obj->pos), entire_contents); } - else - { + else { reprfmt = "<%s is_closed=False fileno=%d access=%s length=%R offset=%R entire_contents=%R ... %R>"; - repr = PyUnicode_FromFormat(reprfmt, tp_name, fd, access_str, - PyLong_FromSize_t(m_obj->size), - PyLong_FromSize_t(m_obj->pos), + repr = PyUnicode_FromFormat(reprfmt, tp_name, fd, access_str, + PyLong_FromSize_t(m_obj->size), + PyLong_FromSize_t(m_obj->pos), PyBytes_FromStringAndSize(&m_obj->data, 50), PyBytes_FromStringAndSize(&m_obj->data + size - 50, 50)); } - } + } return repr; } From d9d8611fe4ba73adec26db152968603a33e56403 Mon Sep 17 00:00:00 2001 From: thautwarm Date: Mon, 15 Oct 2018 14:54:22 +0800 Subject: [PATCH 04/16] follow PEP7 --- Modules/mmapmodule.c | 47 ++++++++++++++++++++++++++++---------------- 1 file changed, 30 insertions(+), 17 deletions(-) diff --git a/Modules/mmapmodule.c b/Modules/mmapmodule.c index 3a1db70795bad0e..9ed0fd04d6b59ed 100644 --- a/Modules/mmapmodule.c +++ b/Modules/mmapmodule.c @@ -716,33 +716,46 @@ mmap__repr__method(PyObject *self) return NULL; } + const char *tp_name = self->ob_type->tp_name; + int fd = m_obj->fd; + if (m_obj -> data == NULL) { + reprfmt = "<%s is_closed=True fileno=%d access=%s>"; - repr = PyUnicode_FromFormat(reprfmt, self->ob_type->tp_name, m_obj->fd, access_str); + repr = PyUnicode_FromFormat(reprfmt, tp_name, fd, access_str); + } else { - const char* tp_name; - int fd; - tp_name = self->ob_type->tp_name; - fd = m_obj->fd; - if (size < 100) - { - reprfmt = "<%s is_closed=False fileno=%d access=%s length=%R offset=%R entire_contents=%R>"; + const char *data = &m_obj->data; + Py_ssize_t pos = m_obj->pos; + + PyObject *length = PyLong_FromSize_t(size); + PyObject *offset = PyLong_FromSize_t(pos); + + if (size < 100) { + + reprfmt = "<%s is_closed=False fileno=%d access=%s length=%R " + "offset=%R entire_contents=%R>"; + PyObject *entire_contents; - entire_contents = PyBytes_FromStringAndSize(&m_obj->data, size); + entire_contents = PyBytes_FromStringAndSize(data, size); + repr = PyUnicode_FromFormat(reprfmt, tp_name, fd, access_str, - PyLong_FromSize_t(m_obj->size), - PyLong_FromSize_t(m_obj->pos), - entire_contents); + length, offset, entire_contents); + } else { - reprfmt = "<%s is_closed=False fileno=%d access=%s length=%R offset=%R entire_contents=%R ... %R>"; + + reprfmt = "<%s is_closed=False fileno=%d access=%s length=%R " + "offset=%R entire_contents=%R ... %R>"; + + PyObject *slice1 = PyBytes_FromStringAndSize(data, 50); + PyObject *slice2 = PyBytes_FromStringAndSize(data + size - 50, + 50); + repr = PyUnicode_FromFormat(reprfmt, tp_name, fd, access_str, - PyLong_FromSize_t(m_obj->size), - PyLong_FromSize_t(m_obj->pos), - PyBytes_FromStringAndSize(&m_obj->data, 50), - PyBytes_FromStringAndSize(&m_obj->data + size - 50, 50)); + length, offset, slice1, slice2); } } From 8f1546a1e398e97458e19ceb3ede5e1b76445ebc Mon Sep 17 00:00:00 2001 From: thautwarm Date: Fri, 19 Apr 2019 11:08:13 +0800 Subject: [PATCH 05/16] remove redundant line wraps and compat MS_WINDOWS --- Modules/mmapmodule.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/Modules/mmapmodule.c b/Modules/mmapmodule.c index 9ed0fd04d6b59ed..4a061310544e7a0 100644 --- a/Modules/mmapmodule.c +++ b/Modules/mmapmodule.c @@ -719,14 +719,16 @@ mmap__repr__method(PyObject *self) const char *tp_name = self->ob_type->tp_name; int fd = m_obj->fd; - if (m_obj -> data == NULL) { - +#ifdef MS_WINDOWS + if (m_obj->map_handle == NULL) +#elif defined(UNIX) + if (m_obj->data == NULL) +#endif + { reprfmt = "<%s is_closed=True fileno=%d access=%s>"; repr = PyUnicode_FromFormat(reprfmt, tp_name, fd, access_str); - } else { - const char *data = &m_obj->data; Py_ssize_t pos = m_obj->pos; @@ -734,7 +736,6 @@ mmap__repr__method(PyObject *self) PyObject *offset = PyLong_FromSize_t(pos); if (size < 100) { - reprfmt = "<%s is_closed=False fileno=%d access=%s length=%R " "offset=%R entire_contents=%R>"; @@ -743,10 +744,8 @@ mmap__repr__method(PyObject *self) repr = PyUnicode_FromFormat(reprfmt, tp_name, fd, access_str, length, offset, entire_contents); - } else { - reprfmt = "<%s is_closed=False fileno=%d access=%s length=%R " "offset=%R entire_contents=%R ... %R>"; From 9d87c5c2d03fe0637bc9e0d9b14353a8dadebd8c Mon Sep 17 00:00:00 2001 From: thautwarm Date: Fri, 19 Apr 2019 11:57:24 +0800 Subject: [PATCH 06/16] fix accessing data from m_obj --- Modules/mmapmodule.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/Modules/mmapmodule.c b/Modules/mmapmodule.c index 4a061310544e7a0..c61b088845bf74d 100644 --- a/Modules/mmapmodule.c +++ b/Modules/mmapmodule.c @@ -729,13 +729,13 @@ mmap__repr__method(PyObject *self) repr = PyUnicode_FromFormat(reprfmt, tp_name, fd, access_str); } else { - const char *data = &m_obj->data; + const char *data = m_obj->data; Py_ssize_t pos = m_obj->pos; PyObject *length = PyLong_FromSize_t(size); PyObject *offset = PyLong_FromSize_t(pos); - if (size < 100) { + if (size < 64) { reprfmt = "<%s is_closed=False fileno=%d access=%s length=%R " "offset=%R entire_contents=%R>"; @@ -749,9 +749,9 @@ mmap__repr__method(PyObject *self) reprfmt = "<%s is_closed=False fileno=%d access=%s length=%R " "offset=%R entire_contents=%R ... %R>"; - PyObject *slice1 = PyBytes_FromStringAndSize(data, 50); - PyObject *slice2 = PyBytes_FromStringAndSize(data + size - 50, - 50); + PyObject *slice1 = PyBytes_FromStringAndSize(data, 32); + PyObject *slice2 = PyBytes_FromStringAndSize(data + size - 32, + 32); repr = PyUnicode_FromFormat(reprfmt, tp_name, fd, access_str, length, offset, slice1, slice2); From 9c77de3d6ed8b6736c1f3bab7e76a0dd543ebbfd Mon Sep 17 00:00:00 2001 From: thautwarm Date: Fri, 19 Apr 2019 12:44:58 +0800 Subject: [PATCH 07/16] add tests --- Lib/test/test_mmap.py | 47 +++++++++++++++++++++++++++++++++++++++++++ Modules/mmapmodule.c | 4 ++-- 2 files changed, 49 insertions(+), 2 deletions(-) diff --git a/Lib/test/test_mmap.py b/Lib/test/test_mmap.py index 246fdf01f9cdea5..4cc1443a94db180 100644 --- a/Lib/test/test_mmap.py +++ b/Lib/test/test_mmap.py @@ -11,6 +11,19 @@ # Skip test if we can't import mmap. mmap = import_module('mmap') +open_mmap_repr_template =\ + "" + +closed_mmap_template =\ + "" + PAGESIZE = mmap.PAGESIZE class MmapTests(unittest.TestCase): @@ -754,6 +767,40 @@ def test_flush_return_value(self): # See bpo-34754 for details. self.assertRaises(OSError, mm.flush, 1, len(b'python')) + def test_open_mmap(self): + def make_entire_contents_keyword(size, data): + if size < 64: + return dict(entire_contents=repr(data)) + return dict(begin=repr(data[:32]), end=repr(data[-32:])) + + for mapsize in (60, 120, 180, 240): + with open(TESTFN, "wb+") as fp: + data = b'a'*mapsize + fp.write(data) + fp.flush() + + with mmap.mmap(fp.fileno(), mapsize) as mm: + mm = mmap.mmap(fp.fileno(), mapsize) + repr_str = open_tiny_mmap_repr_template.format( + fileno=fp.fileno(), + access="ACCESS_DEFAULT", + size=mapsize, + offset=0, + **make_entire_contents_keyword(mapsize, data) + ) + self.assertEqual(repr(mm), repr_str) + + for offset in [mapsize//5, mapsize//4, + mapsize//3, mapsize//2]: + mm.seek(offset) + repr_str = open_tiny_mmap_repr_template.format( + fileno=fp.fileno(), + access="ACCESS_DEFAULT", + size=mapsize, + offset=offset, + **make_entire_contents_keyword(mapsize, data) + ) + self.assertEqual(repr(mm), repr_str) class LargeMmapTests(unittest.TestCase): diff --git a/Modules/mmapmodule.c b/Modules/mmapmodule.c index c61b088845bf74d..4525002e7ede7a8 100644 --- a/Modules/mmapmodule.c +++ b/Modules/mmapmodule.c @@ -736,7 +736,7 @@ mmap__repr__method(PyObject *self) PyObject *offset = PyLong_FromSize_t(pos); if (size < 64) { - reprfmt = "<%s is_closed=False fileno=%d access=%s length=%R " + reprfmt = "<%s is_closed=False fileno=%d access=%s size=%R " "offset=%R entire_contents=%R>"; PyObject *entire_contents; @@ -746,7 +746,7 @@ mmap__repr__method(PyObject *self) length, offset, entire_contents); } else { - reprfmt = "<%s is_closed=False fileno=%d access=%s length=%R " + reprfmt = "<%s is_closed=False fileno=%d access=%s size=%R " "offset=%R entire_contents=%R ... %R>"; PyObject *slice1 = PyBytes_FromStringAndSize(data, 32); From 3226a355a3ac5679b89de71c50a14fbe7d0c83bf Mon Sep 17 00:00:00 2001 From: thautwarm Date: Fri, 19 Apr 2019 14:51:52 +0800 Subject: [PATCH 08/16] compat windows and add exhaustive tests --- Lib/test/test_mmap.py | 67 +++++++++++++++++++++---------------------- Modules/mmapmodule.c | 13 +++++---- 2 files changed, 39 insertions(+), 41 deletions(-) diff --git a/Lib/test/test_mmap.py b/Lib/test/test_mmap.py index 4cc1443a94db180..d10a421ebbbfab9 100644 --- a/Lib/test/test_mmap.py +++ b/Lib/test/test_mmap.py @@ -11,18 +11,15 @@ # Skip test if we can't import mmap. mmap = import_module('mmap') -open_mmap_repr_template =\ - "\S+) size=(?P\d+) " + r"offset=(?P\d+) " + r"entire_contents=(?P[\w\W]+)>") -open_tiny_mmap_repr_template =\ - "" - -closed_mmap_template =\ - "" +closed_mmap_repr_pat = re.compile( + r"\S+)>") PAGESIZE = mmap.PAGESIZE @@ -768,39 +765,39 @@ def test_flush_return_value(self): self.assertRaises(OSError, mm.flush, 1, len(b'python')) def test_open_mmap(self): - def make_entire_contents_keyword(size, data): + def show_entire_contents(size, data): if size < 64: - return dict(entire_contents=repr(data)) - return dict(begin=repr(data[:32]), end=repr(data[-32:])) + return repr(data) + return repr(data[:32]) + ' ... ' + repr(data[-32:]) for mapsize in (60, 120, 180, 240): + + accesses = ('ACCESS_DEFAULT', 'ACCESS_READ', + 'ACCESS_COPY', 'ACCESS_WRITE') + + offsets = (0, mapsize//5, mapsize//4, mapsize//3, mapsize//2) + with open(TESTFN, "wb+") as fp: data = b'a'*mapsize fp.write(data) fp.flush() - with mmap.mmap(fp.fileno(), mapsize) as mm: - mm = mmap.mmap(fp.fileno(), mapsize) - repr_str = open_tiny_mmap_repr_template.format( - fileno=fp.fileno(), - access="ACCESS_DEFAULT", - size=mapsize, - offset=0, - **make_entire_contents_keyword(mapsize, data) - ) - self.assertEqual(repr(mm), repr_str) - - for offset in [mapsize//5, mapsize//4, - mapsize//3, mapsize//2]: + for access, offset in itertools.product(accesses, offsets): + accint = getattr(mmap, access) + with mmap.mmap(fp.fileno(), mapsize, access=accint) as mm: mm.seek(offset) - repr_str = open_tiny_mmap_repr_template.format( - fileno=fp.fileno(), - access="ACCESS_DEFAULT", - size=mapsize, - offset=offset, - **make_entire_contents_keyword(mapsize, data) - ) - self.assertEqual(repr(mm), repr_str) + match = open_mmap_repr_pat.match(repr(mm)) + self.assertIsNotNone(match) + self.assertEqual(match.group('access'), access) + self.assertEqual(match.group('size'), str(mapsize)) + self.assertEqual(match.group('offset'), str(offset)) + self.assertEqual(match.group('entire_contents'), + show_entire_contents(mapsize, data)) + + match = closed_mmap_repr_pat.match(repr(mm)) + self.assertIsNotNone(match) + self.assertEqual(match.group('access'), access) + class LargeMmapTests(unittest.TestCase): diff --git a/Modules/mmapmodule.c b/Modules/mmapmodule.c index 4525002e7ede7a8..01ab4be180f7b62 100644 --- a/Modules/mmapmodule.c +++ b/Modules/mmapmodule.c @@ -692,8 +692,6 @@ mmap__repr__method(PyObject *self) const char *reprfmt; const char *access_str; - Py_ssize_t size = m_obj->size; - switch (m_obj->access) { case ACCESS_DEFAULT: access_str = "ACCESS_DEFAULT"; @@ -716,17 +714,20 @@ mmap__repr__method(PyObject *self) return NULL; } + Py_ssize_t size = m_obj->size; const char *tp_name = self->ob_type->tp_name; - int fd = m_obj->fd; + #ifdef MS_WINDOWS + int fileno = m_obj->file_handle; if (m_obj->map_handle == NULL) #elif defined(UNIX) + int fileno = m_obj->fd; if (m_obj->data == NULL) #endif { reprfmt = "<%s is_closed=True fileno=%d access=%s>"; - repr = PyUnicode_FromFormat(reprfmt, tp_name, fd, access_str); + repr = PyUnicode_FromFormat(reprfmt, tp_name, fileno, access_str); } else { const char *data = m_obj->data; @@ -742,7 +743,7 @@ mmap__repr__method(PyObject *self) PyObject *entire_contents; entire_contents = PyBytes_FromStringAndSize(data, size); - repr = PyUnicode_FromFormat(reprfmt, tp_name, fd, access_str, + repr = PyUnicode_FromFormat(reprfmt, tp_name, fileno, access_str, length, offset, entire_contents); } else { @@ -753,7 +754,7 @@ mmap__repr__method(PyObject *self) PyObject *slice2 = PyBytes_FromStringAndSize(data + size - 32, 32); - repr = PyUnicode_FromFormat(reprfmt, tp_name, fd, access_str, + repr = PyUnicode_FromFormat(reprfmt, tp_name, fileno, access_str, length, offset, slice1, slice2); } } From e4c2440d91d069f468fbc977a3d92af3f508fade Mon Sep 17 00:00:00 2001 From: thautwarm Date: Fri, 26 Apr 2019 21:11:58 +0800 Subject: [PATCH 09/16] follow Zhang's guides to refine codes --- Lib/test/test_mmap.py | 2 +- Modules/mmapmodule.c | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/Lib/test/test_mmap.py b/Lib/test/test_mmap.py index d10a421ebbbfab9..a3e947badadabcc 100644 --- a/Lib/test/test_mmap.py +++ b/Lib/test/test_mmap.py @@ -764,7 +764,7 @@ def test_flush_return_value(self): # See bpo-34754 for details. self.assertRaises(OSError, mm.flush, 1, len(b'python')) - def test_open_mmap(self): + def test_repr(self): def show_entire_contents(size, data): if size < 64: return repr(data) diff --git a/Modules/mmapmodule.c b/Modules/mmapmodule.c index 01ab4be180f7b62..f8fe329a3e7ee67 100644 --- a/Modules/mmapmodule.c +++ b/Modules/mmapmodule.c @@ -710,12 +710,13 @@ mmap__repr__method(PyObject *self) break; default: - PyErr_SetString(PyExc_IOError, "Unknown access mode for mmap object."); + // should not get here + assert(false); return NULL; } Py_ssize_t size = m_obj->size; - const char *tp_name = self->ob_type->tp_name; + const char *tp_name = Py_TYPE(self)->tp_name; #ifdef MS_WINDOWS From ff18f7da79c2b4439f0bd9cd17074dbe580be7fa Mon Sep 17 00:00:00 2001 From: thautwarm Date: Fri, 26 Apr 2019 21:45:16 +0800 Subject: [PATCH 10/16] fix tests --- Modules/mmapmodule.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Modules/mmapmodule.c b/Modules/mmapmodule.c index f8fe329a3e7ee67..efc9b28a5de3712 100644 --- a/Modules/mmapmodule.c +++ b/Modules/mmapmodule.c @@ -711,7 +711,7 @@ mmap__repr__method(PyObject *self) default: // should not get here - assert(false); + assert(0); return NULL; } From 3b1d544c80ff131b4e2ad1d5ac984fca0d45d1d3 Mon Sep 17 00:00:00 2001 From: thautwarm Date: Mon, 29 Apr 2019 10:37:29 +0800 Subject: [PATCH 11/16] avoid showing entire_contents according to core devs' points of view --- Lib/test/test_mmap.py | 14 +++----------- Modules/mmapmodule.c | 21 +++++++-------------- 2 files changed, 10 insertions(+), 25 deletions(-) diff --git a/Lib/test/test_mmap.py b/Lib/test/test_mmap.py index a3e947badadabcc..aa63ed3b0942b0c 100644 --- a/Lib/test/test_mmap.py +++ b/Lib/test/test_mmap.py @@ -12,13 +12,12 @@ mmap = import_module('mmap') open_mmap_repr_pat = re.compile( - r"\S+) size=(?P\d+) " - r"offset=(?P\d+) " - r"entire_contents=(?P[\w\W]+)>") + r"offset=(?P\d+)>") closed_mmap_repr_pat = re.compile( - r"\S+)>") PAGESIZE = mmap.PAGESIZE @@ -765,11 +764,6 @@ def test_flush_return_value(self): self.assertRaises(OSError, mm.flush, 1, len(b'python')) def test_repr(self): - def show_entire_contents(size, data): - if size < 64: - return repr(data) - return repr(data[:32]) + ' ... ' + repr(data[-32:]) - for mapsize in (60, 120, 180, 240): accesses = ('ACCESS_DEFAULT', 'ACCESS_READ', @@ -791,8 +785,6 @@ def show_entire_contents(size, data): self.assertEqual(match.group('access'), access) self.assertEqual(match.group('size'), str(mapsize)) self.assertEqual(match.group('offset'), str(offset)) - self.assertEqual(match.group('entire_contents'), - show_entire_contents(mapsize, data)) match = closed_mmap_repr_pat.match(repr(mm)) self.assertIsNotNone(match) diff --git a/Modules/mmapmodule.c b/Modules/mmapmodule.c index efc9b28a5de3712..e6d569e9f01b62d 100644 --- a/Modules/mmapmodule.c +++ b/Modules/mmapmodule.c @@ -727,7 +727,7 @@ mmap__repr__method(PyObject *self) if (m_obj->data == NULL) #endif { - reprfmt = "<%s is_closed=True fileno=%d access=%s>"; + reprfmt = "<%s closed=True fileno=%d access=%s>"; repr = PyUnicode_FromFormat(reprfmt, tp_name, fileno, access_str); } else { @@ -738,25 +738,18 @@ mmap__repr__method(PyObject *self) PyObject *offset = PyLong_FromSize_t(pos); if (size < 64) { - reprfmt = "<%s is_closed=False fileno=%d access=%s size=%R " - "offset=%R entire_contents=%R>"; - - PyObject *entire_contents; - entire_contents = PyBytes_FromStringAndSize(data, size); + reprfmt = "<%s closed=False fileno=%d access=%s size=%R " + "offset=%R>"; repr = PyUnicode_FromFormat(reprfmt, tp_name, fileno, access_str, - length, offset, entire_contents); + length, offset); } else { - reprfmt = "<%s is_closed=False fileno=%d access=%s size=%R " - "offset=%R entire_contents=%R ... %R>"; - - PyObject *slice1 = PyBytes_FromStringAndSize(data, 32); - PyObject *slice2 = PyBytes_FromStringAndSize(data + size - 32, - 32); + reprfmt = "<%s closed=False fileno=%d access=%s size=%R " + "offset=%R>"; repr = PyUnicode_FromFormat(reprfmt, tp_name, fileno, access_str, - length, offset, slice1, slice2); + length, offset); } } From d5ffb479323b5ae2a157cd6442a762cb59405209 Mon Sep 17 00:00:00 2001 From: thautwarm Date: Mon, 29 Apr 2019 17:57:31 +0800 Subject: [PATCH 12/16] test data with long int size & refine repr format args & remove redundant codes --- Lib/test/test_mmap.py | 3 +-- Modules/mmapmodule.c | 21 ++++----------------- 2 files changed, 5 insertions(+), 19 deletions(-) diff --git a/Lib/test/test_mmap.py b/Lib/test/test_mmap.py index aa63ed3b0942b0c..fcd297f294e1f6c 100644 --- a/Lib/test/test_mmap.py +++ b/Lib/test/test_mmap.py @@ -764,8 +764,7 @@ def test_flush_return_value(self): self.assertRaises(OSError, mm.flush, 1, len(b'python')) def test_repr(self): - for mapsize in (60, 120, 180, 240): - + for mapsize in (60, 120, 180, 240, 180 * 240, 240 * 120 * 60): accesses = ('ACCESS_DEFAULT', 'ACCESS_READ', 'ACCESS_COPY', 'ACCESS_WRITE') diff --git a/Modules/mmapmodule.c b/Modules/mmapmodule.c index e6d569e9f01b62d..9a9945ebe9363d5 100644 --- a/Modules/mmapmodule.c +++ b/Modules/mmapmodule.c @@ -731,26 +731,13 @@ mmap__repr__method(PyObject *self) repr = PyUnicode_FromFormat(reprfmt, tp_name, fileno, access_str); } else { - const char *data = m_obj->data; Py_ssize_t pos = m_obj->pos; - PyObject *length = PyLong_FromSize_t(size); - PyObject *offset = PyLong_FromSize_t(pos); + reprfmt = "<%s closed=False fileno=%d access=%s size=%ld " + "offset=%ld>"; - if (size < 64) { - reprfmt = "<%s closed=False fileno=%d access=%s size=%R " - "offset=%R>"; - - repr = PyUnicode_FromFormat(reprfmt, tp_name, fileno, access_str, - length, offset); - } - else { - reprfmt = "<%s closed=False fileno=%d access=%s size=%R " - "offset=%R>"; - - repr = PyUnicode_FromFormat(reprfmt, tp_name, fileno, access_str, - length, offset); - } + repr = PyUnicode_FromFormat(reprfmt, tp_name, fileno, access_str, + size, pos); } return repr; From 09b9554b3cc5d16f2ad89dfd969ba7dc567b4924 Mon Sep 17 00:00:00 2001 From: thautwarm Date: Mon, 29 Apr 2019 18:08:32 +0800 Subject: [PATCH 13/16] invalid access now causes Py_UNREACHABLE --- Modules/mmapmodule.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/Modules/mmapmodule.c b/Modules/mmapmodule.c index 9a9945ebe9363d5..115382ad3500cba 100644 --- a/Modules/mmapmodule.c +++ b/Modules/mmapmodule.c @@ -710,9 +710,7 @@ mmap__repr__method(PyObject *self) break; default: - // should not get here - assert(0); - return NULL; + Py_UNREACHABLE(); } Py_ssize_t size = m_obj->size; From a1b589f17c4c25107872cedcfd2618211150c68c Mon Sep 17 00:00:00 2001 From: thautwarm Date: Wed, 1 May 2019 00:04:39 +0800 Subject: [PATCH 14/16] remove 'fileno' due to code review and discussion & refine wording(clarify usage of 'pos', 'offset' and 'length') --- Lib/test/test_mmap.py | 36 +++++++++++++++++++++++------------- Modules/mmapmodule.c | 21 ++++++++------------- 2 files changed, 31 insertions(+), 26 deletions(-) diff --git a/Lib/test/test_mmap.py b/Lib/test/test_mmap.py index fcd297f294e1f6c..97ca458459f4106 100644 --- a/Lib/test/test_mmap.py +++ b/Lib/test/test_mmap.py @@ -12,12 +12,14 @@ mmap = import_module('mmap') open_mmap_repr_pat = re.compile( - r"\S+) size=(?P\d+) " + r"\S+) " + r"length=(?P\d+) " + r"pos=(?P\d+) " r"offset=(?P\d+)>") closed_mmap_repr_pat = re.compile( - r"\S+)>") PAGESIZE = mmap.PAGESIZE @@ -764,25 +766,33 @@ def test_flush_return_value(self): self.assertRaises(OSError, mm.flush, 1, len(b'python')) def test_repr(self): - for mapsize in (60, 120, 180, 240, 180 * 240, 240 * 120 * 60): - accesses = ('ACCESS_DEFAULT', 'ACCESS_READ', - 'ACCESS_COPY', 'ACCESS_WRITE') - - offsets = (0, mapsize//5, mapsize//4, mapsize//3, mapsize//2) + mapsizes = (50, 100, 1_000, 1_000_000, 10_000_000) + offsets = tuple((mapsize // 2 // PAGESIZE) * PAGESIZE for mapsize in mapsizes) + for offset, mapsize in zip(offsets, mapsizes): + data = b'a' * mapsize + length = mapsize - offset + accesses = ('ACCESS_DEFAULT', 'ACCESS_READ', + 'ACCESS_COPY', 'ACCESS_WRITE') + positions = (0, length//10, length//5, length//4) with open(TESTFN, "wb+") as fp: - data = b'a'*mapsize fp.write(data) fp.flush() - for access, offset in itertools.product(accesses, offsets): + for access, pos in itertools.product(accesses, positions): accint = getattr(mmap, access) - with mmap.mmap(fp.fileno(), mapsize, access=accint) as mm: - mm.seek(offset) + + with mmap.mmap(fp.fileno(), + length, + access=accint, + offset=offset) as mm: + + mm.seek(pos) match = open_mmap_repr_pat.match(repr(mm)) self.assertIsNotNone(match) self.assertEqual(match.group('access'), access) - self.assertEqual(match.group('size'), str(mapsize)) + self.assertEqual(match.group('length'), str(length)) + self.assertEqual(match.group('pos'), str(pos)) self.assertEqual(match.group('offset'), str(offset)) match = closed_mmap_repr_pat.match(repr(mm)) diff --git a/Modules/mmapmodule.c b/Modules/mmapmodule.c index 115382ad3500cba..7aaa0e5eac7bb01 100644 --- a/Modules/mmapmodule.c +++ b/Modules/mmapmodule.c @@ -713,29 +713,24 @@ mmap__repr__method(PyObject *self) Py_UNREACHABLE(); } - Py_ssize_t size = m_obj->size; const char *tp_name = Py_TYPE(self)->tp_name; - #ifdef MS_WINDOWS - int fileno = m_obj->file_handle; if (m_obj->map_handle == NULL) #elif defined(UNIX) - int fileno = m_obj->fd; if (m_obj->data == NULL) #endif { - reprfmt = "<%s closed=True fileno=%d access=%s>"; - repr = PyUnicode_FromFormat(reprfmt, tp_name, fileno, access_str); + reprfmt = "<%s closed=True access=%s>"; + repr = PyUnicode_FromFormat(reprfmt, tp_name, access_str); } else { - Py_ssize_t pos = m_obj->pos; - - reprfmt = "<%s closed=False fileno=%d access=%s size=%ld " - "offset=%ld>"; - - repr = PyUnicode_FromFormat(reprfmt, tp_name, fileno, access_str, - size, pos); + PyObject *length = PyLong_FromSize_t(m_obj->size); + PyObject *pos = PyLong_FromSize_t(m_obj->pos); + PyObject *offset = PyLong_FromSize_t((Py_ssize_t)m_obj->offset); + reprfmt = "<%s closed=False access=%s length=%R pos=%R offset=%R>"; + repr = PyUnicode_FromFormat(reprfmt, tp_name, access_str, + length, pos, offset); } return repr; From 39d84a13f0b462f9d8bc84e200e0b3328ef76865 Mon Sep 17 00:00:00 2001 From: Xiang Zhang Date: Mon, 14 Oct 2019 00:22:42 +0800 Subject: [PATCH 15/16] adjusts --- Lib/test/test_mmap.py | 26 ++++------- Modules/mmapmodule.c | 102 +++++++++++++++++++----------------------- 2 files changed, 54 insertions(+), 74 deletions(-) diff --git a/Lib/test/test_mmap.py b/Lib/test/test_mmap.py index 99081a5d2c24190..d8b1a5ee8cc385a 100644 --- a/Lib/test/test_mmap.py +++ b/Lib/test/test_mmap.py @@ -11,17 +11,6 @@ # Skip test if we can't import mmap. mmap = import_module('mmap') -open_mmap_repr_pat = re.compile( - r"\S+) " - r"length=(?P\d+) " - r"pos=(?P\d+) " - r"offset=(?P\d+)>") - -closed_mmap_repr_pat = re.compile( - r"\S+)>") - PAGESIZE = mmap.PAGESIZE @@ -752,27 +741,30 @@ def test_flush_return_value(self): self.assertRaises(OSError, mm.flush, 1, len(b'python')) def test_repr(self): + open_mmap_repr_pat = re.compile( + r"\S+), " + r"length=(?P\d+), " + r"pos=(?P\d+), " + r"offset=(?P\d+)>") + closed_mmap_repr_pat = re.compile(r"") mapsizes = (50, 100, 1_000, 1_000_000, 10_000_000) - offsets = tuple((mapsize // 2 // PAGESIZE) * PAGESIZE for mapsize in mapsizes) + offsets = tuple((mapsize//2//PAGESIZE) * PAGESIZE for mapsize in mapsizes) for offset, mapsize in zip(offsets, mapsizes): data = b'a' * mapsize length = mapsize - offset accesses = ('ACCESS_DEFAULT', 'ACCESS_READ', 'ACCESS_COPY', 'ACCESS_WRITE') positions = (0, length//10, length//5, length//4) - with open(TESTFN, "wb+") as fp: fp.write(data) fp.flush() - for access, pos in itertools.product(accesses, positions): accint = getattr(mmap, access) - with mmap.mmap(fp.fileno(), length, access=accint, offset=offset) as mm: - mm.seek(pos) match = open_mmap_repr_pat.match(repr(mm)) self.assertIsNotNone(match) @@ -780,10 +772,8 @@ def test_repr(self): self.assertEqual(match.group('length'), str(length)) self.assertEqual(match.group('pos'), str(pos)) self.assertEqual(match.group('offset'), str(offset)) - match = closed_mmap_repr_pat.match(repr(mm)) self.assertIsNotNone(match) - self.assertEqual(match.group('access'), access) @unittest.skipUnless(hasattr(mmap.mmap, 'madvise'), 'needs madvise') def test_madvise(self): diff --git a/Modules/mmapmodule.c b/Modules/mmapmodule.c index 0129fbb6911777b..a3b9e4df95305e0 100644 --- a/Modules/mmapmodule.c +++ b/Modules/mmapmodule.c @@ -698,55 +698,46 @@ mmap__exit__method(PyObject *self, PyObject *args) static PyObject * mmap__repr__method(PyObject *self) { - PyObject *repr; - mmap_object *m_obj; - m_obj = (mmap_object *) self; - - const char *reprfmt; - const char *access_str; - - switch (m_obj->access) { - case ACCESS_DEFAULT: - access_str = "ACCESS_DEFAULT"; - break; - - case ACCESS_READ: - access_str = "ACCESS_READ"; - break; - - case ACCESS_WRITE: - access_str = "ACCESS_WRITE"; - break; - - case ACCESS_COPY: - access_str = "ACCESS_COPY"; - break; - - default: - Py_UNREACHABLE(); - } - - const char *tp_name = Py_TYPE(self)->tp_name; + mmap_object *mobj = (mmap_object *)self; #ifdef MS_WINDOWS - if (m_obj->map_handle == NULL) +#define _Py_FORMAT_OFFSET "lld" + if (mobj->map_handle == NULL) #elif defined(UNIX) - if (m_obj->data == NULL) +# ifdef HAVE_LARGEFILE_SUPPORT +# define _Py_FORMAT_OFFSET "lld" +# else +# define _Py_FORMAT_OFFSET "ld" +# endif + if (mobj->data == NULL) #endif { - reprfmt = "<%s closed=True access=%s>"; - repr = PyUnicode_FromFormat(reprfmt, tp_name, access_str); - } - else { - PyObject *length = PyLong_FromSize_t(m_obj->size); - PyObject *pos = PyLong_FromSize_t(m_obj->pos); - PyObject *offset = PyLong_FromSize_t((Py_ssize_t)m_obj->offset); - reprfmt = "<%s closed=False access=%s length=%R pos=%R offset=%R>"; - repr = PyUnicode_FromFormat(reprfmt, tp_name, access_str, - length, pos, offset); - } + return PyUnicode_FromFormat("<%s closed=True>", Py_TYPE(self)->tp_name); + } else { + const char *access_str; + + switch (mobj->access) { + case ACCESS_DEFAULT: + access_str = "ACCESS_DEFAULT"; + break; + case ACCESS_READ: + access_str = "ACCESS_READ"; + break; + case ACCESS_WRITE: + access_str = "ACCESS_WRITE"; + break; + case ACCESS_COPY: + access_str = "ACCESS_COPY"; + break; + default: + Py_UNREACHABLE(); + } - return repr; + return PyUnicode_FromFormat("<%s closed=False, access=%s, length=%zd, " + "pos=%zd, offset=%" _Py_FORMAT_OFFSET ">", + Py_TYPE(self)->tp_name, access_str, + mobj->size, mobj->pos, mobj->offset); + } } #ifdef MS_WINDOWS @@ -822,7 +813,6 @@ static struct PyMethodDef mmap_object_methods[] = { {"write_byte", (PyCFunction) mmap_write_byte_method, METH_VARARGS}, {"__enter__", (PyCFunction) mmap__enter__method, METH_NOARGS}, {"__exit__", (PyCFunction) mmap__exit__method, METH_VARARGS}, - {"__repr__", (PyCFunction) mmap__repr__method, METH_NOARGS}, #ifdef MS_WINDOWS {"__sizeof__", (PyCFunction) mmap__sizeof__method, METH_NOARGS}, #endif @@ -1099,23 +1089,23 @@ static PyTypeObject mmap_object_type = { sizeof(mmap_object), /* tp_basicsize */ 0, /* tp_itemsize */ /* methods */ - (destructor) mmap_object_dealloc, /* tp_dealloc */ + (destructor)mmap_object_dealloc, /* tp_dealloc */ 0, /* tp_vectorcall_offset */ 0, /* tp_getattr */ 0, /* tp_setattr */ 0, /* tp_as_async */ - 0, /* tp_repr */ + (reprfunc)mmap__repr__method, /* tp_repr */ 0, /* tp_as_number */ - &mmap_as_sequence, /*tp_as_sequence*/ - &mmap_as_mapping, /*tp_as_mapping*/ - 0, /*tp_hash*/ - 0, /*tp_call*/ - 0, /*tp_str*/ - PyObject_GenericGetAttr, /*tp_getattro*/ - 0, /*tp_setattro*/ - &mmap_as_buffer, /*tp_as_buffer*/ - Py_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE, /*tp_flags*/ - mmap_doc, /*tp_doc*/ + &mmap_as_sequence, /* tp_as_sequence */ + &mmap_as_mapping, /* tp_as_mapping */ + 0, /* tp_hash */ + 0, /* tp_call */ + 0, /* tp_str */ + PyObject_GenericGetAttr, /* tp_getattro */ + 0, /* tp_setattro */ + &mmap_as_buffer, /* tp_as_buffer */ + Py_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE, /* tp_flags */ + mmap_doc, /* tp_doc */ 0, /* tp_traverse */ 0, /* tp_clear */ 0, /* tp_richcompare */ From bf68fc7d2fd50f94cb1da5381bf0f442b8f6b8f2 Mon Sep 17 00:00:00 2001 From: Xiang Zhang Date: Mon, 14 Oct 2019 01:15:54 +0800 Subject: [PATCH 16/16] replace PAGESIZE with ALLOCATIONGRANULARITY --- Lib/test/test_mmap.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Lib/test/test_mmap.py b/Lib/test/test_mmap.py index d8b1a5ee8cc385a..5400f25f50800b6 100644 --- a/Lib/test/test_mmap.py +++ b/Lib/test/test_mmap.py @@ -749,7 +749,8 @@ def test_repr(self): r"offset=(?P\d+)>") closed_mmap_repr_pat = re.compile(r"") mapsizes = (50, 100, 1_000, 1_000_000, 10_000_000) - offsets = tuple((mapsize//2//PAGESIZE) * PAGESIZE for mapsize in mapsizes) + offsets = tuple((mapsize // 2 // mmap.ALLOCATIONGRANULARITY) + * mmap.ALLOCATIONGRANULARITY for mapsize in mapsizes) for offset, mapsize in zip(offsets, mapsizes): data = b'a' * mapsize length = mapsize - offset