bpo-34953: Implement `mmap.mmap.repr` #9891

thautwarm · 2018-10-15T11:40:01Z

Add pretty __repr__ to mmap.mmap and its subtypes.

import mmap
f = open('../a.txt', 'r+b')
mm = mmap.mmap(f.fileno(), 0)
print(mm) # =><mmap.mmap is_closed=False fileno=4 access=ACCESS_DEFAULT length=4 offset=0 entire_contents=b'\x00\xf0\x93S'>
mm.close()
print(mm) # => <mmap.mmap is_closed=True fileno=-1 access=ACCESS_DEFAULT>

If the contents are too long, only the first 50 32 and the last 50 32 would be presented:

... content=b'This is my' ... b'file.'>

https://bugs.python.org/issue34953

the-knights-who-say-ni · 2018-10-15T11:40:04Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately our records indicate you have not signed the CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for your contribution, we look forward to reviewing it!

zhangyangyu · 2018-10-16T03:08:17Z

Modules/mmapmodule.c

+
+        reprfmt = "<%s is_closed=True fileno=%d access=%s>";
+        repr = PyUnicode_FromFormat(reprfmt, tp_name, fd, access_str);
+


No need for the wrapped empty lines. And most C APIs could fail and we need to check it.

zhangyangyu · 2018-10-16T03:11:19Z

Please add some tests.

thautwarm · 2018-10-16T06:28:22Z

Okay, thanks. I'll remove wrapped empty lines and add some tests.

csabella · 2019-04-10T20:36:47Z

@thautwarm Did you have questions about creating the tests for this?

thautwarm · 2019-04-10T20:41:45Z

@thautwarm Did you have questions about creating the tests for this?

Sorry, I'll have it out before the next week.

thautwarm · 2019-04-19T07:27:54Z

Sorry for my late commits. May you please review my codes @zhangyangyu when your schedule is available?

zhangyangyu · 2019-04-26T07:30:47Z

Modules/mmapmodule.c

+            break;
+
+        default:
+            PyErr_SetString(PyExc_IOError, "Unknown access mode for mmap object.");


Not possible to reach here IMHO. And IOError is not a right exception type here. I think no need to raise a exception here, a simple comment // should not get here, or an assertion assert(null), or access_str = "UNKNOWN" is better.

Understood. I'll insert an assert here.

zhangyangyu · 2019-04-26T07:37:00Z

Modules/mmapmodule.c

+    }
+
+    Py_ssize_t size = m_obj->size;
+    const char *tp_name = self->ob_type->tp_name;


Could use Py_TYPE.

Okay, I'll assemble your notes and make a commit tonight.

zhangyangyu · 2019-04-26T07:47:09Z

Modules/mmapmodule.c

+
+            repr = PyUnicode_FromFormat(reprfmt, tp_name, fileno, access_str,
+                                        length, offset, slice1, slice2);
+        }


I don't think this part is necessary. IMHO we should not show the content in repr.

The original proposer of this said he felt like to have these ones:

Length

Whether it's a file or anonymous memory

The entire contents

The clipped contents

Whether it's opened or closed

The access type

I think having entire contents is infeasible, but others seem to be reasonable.

Lib/test/test_mmap.py

thautwarm · 2019-04-26T13:46:57Z

assert(false) passed my local compilation, but fail in azure pipeline.

zhangyangyu · 2019-04-29T08:38:05Z

Modules/mmapmodule.c

+        default:
+            // should not get here
+            assert(0);
+            return NULL;


I remember we get a new Py_UNREACHABLE macro, we could use it here, no need to return.

zhangyangyu · 2019-04-29T08:41:12Z

Modules/mmapmodule.c

+        Py_ssize_t pos = m_obj->pos;
+
+        PyObject *length = PyLong_FromSize_t(size);
+        PyObject *offset = PyLong_FromSize_t(pos);


C APIs could fail. You need to check here and if they fail, let the function returns and propagates exceptions. And PyUnicode_FromFormat directly support format characters for these basic types, do we still need to do this?

Yeah we don't really need this. I used PyObject for arbitrary precision, but it seems that mmap_object supports only long integer size. Thus I use %lu instead.

pos and size are both of type Py_ssize_t, you don't need to care what actually precision it is for represention case, the format function will take care of it. %lu is not the exact format here, checkout https://docs.python.org/3/c-api/unicode.html#c.PyUnicode_FromFormat.

Okay, thanks for this!

zhangyangyu · 2019-04-29T08:47:36Z

Modules/mmapmodule.c

+
+            repr = PyUnicode_FromFormat(reprfmt, tp_name, fileno, access_str,
+                                        length, offset);
+        }


Hmm, no difference between these. I'd suggest make closed a local variable and the logic could be clearer.

There is a difference, that when data is not available, getting offset could be insecure.

Sorry, I don't get it. Why could it be insecure? After new_mmap_object they are all initialized right? What I am missing here? And if they are secure, I don't think we need to distingiush them between closed and open status, just show whatever they are. One more thing, why leave out the actual offset property and make pos the offset? It's confusing.

First, I'd say sorry for offset should be pos.

At the first beginning I mean if mmap gets closed, the data of mmap_object has already been NULL. In this situation why should we allow uers to get a few outdated members like pos or size? I know people might feel like to ask a closed mmap for the historical records so I think you're right in that case, but in fact, the tell method and size method just raise exception PyErr_SetString(PyExc_ValueError, "mmap closed or invalid"):

tell: https://github.com/python/cpython/blob/master/Modules/mmapmodule.c#L567

size: https://github.com/python/cpython/blob/master/Modules/mmapmodule.c#L417

So there's no way to get size and pos from a mmap object when it has been closed, why should we break this in just repr method? That's also what now motivates me to still distinguish closed status from the open one.

Hmm, I know why I once used offset now..
It's the argument of mmap constructor: https://github.com/python/cpython/blob/master/Modules/mmapmodule.c#L1058

Also, length is used there instead of size.

I think we should do more to make it consistent in both low level(C data structures) and high level(interfaces for Python).
It might be better to change size to length and consist using offset via

https://github.com/python/cpython/blob/master/Modules/mmapmodule.c#L1144

https://github.com/python/cpython/blob/master/Modules/mmapmodule.c#L1133

The wording seems to be a thing, e.t.c, in previous codes there're already phrases like length of mmap, file size or memory offset:
https://github.com/python/cpython/blob/master/Modules/mmapmodule.c#L1133-L1145

Makes sense. Some properties are meaningless when closed, we don't need them, I mean pos. For others, I'd suggest just using the constructor arguments, length is length, pos is pos, offset is offset. Don't use size please since it's confusing, it's stored internally as a caculated field but unforunately there is also a size() method returns something different. As for fileno, I am okay to omit it for now. Actually no pos is also okay for me.

zhangyangyu · 2019-04-29T08:56:43Z

Modules/mmapmodule.c

+        repr = PyUnicode_FromFormat(reprfmt, tp_name, fileno, access_str);
+    }
+    else {
+        const char *data = m_obj->data;


…dant codes

zhangyangyu · 2019-04-29T12:45:54Z

Modules/mmapmodule.c

+
+
+#ifdef MS_WINDOWS
+    int fileno = m_obj->file_handle;


This doesn't seem right. HANDLE on windows is a pointer. Seems no official way to stringify HANDLE, it's okay for me to omit it on Windows.

In fact, fileno is quite inaccurate here. However I've already found a use case that giving fileno here as an integer will make sense, say, people might try to parse some repr string by regex, if we remove fileno here, it can cause some incompatibility between Unix and Windows.

What deserves to be mentioned is, there's no way to get fileno from mmap regardless of we're in Windows or Unix, so I think it'll be appealing to remove fileno here(although the author of this bpo-34953 has requested it and fileno in Unix does have useful semantics).

…arify usage of 'pos', 'offset' and 'length')

zhangyangyu · 2019-05-05T05:59:05Z

Modules/mmapmodule.c

+        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);


It should be reprfmt = "<%s closed=False, pos=%zd, length=%zd, offset=%ld/%lld, access=%s>", then PyUnicode_FromFormat(reprfmt, tp_name, pos, length, offset, access_str). You don't need to call the PyLong_FromSize_t yourself. More, I suggest add comma between properties. And be careful to decide which format character offset uses, you'd better don't cast anyway, you need to decide using macros, check new_mmap_object.

Okay, but for the , separator, I'm not sure yet. In fact there're both styles(<a b c> like fileio and <a, b, c>) in repr.

For offset, I do want to use PyLong_FromOff_t because off_t type is a bit tricky in cross-platform issues.
If I include another header file for only one function, it's much too cumbersome; but if I don't, I have to do what that've been done in _iomodule.h once again.

For comma it's just preference, I prefer it. There is no PyLong_FromOff_t and don't include _iomodule.h, usually we don't include dependency between modules. Use macros to determine the right format character, refer new_mmap_object please. Don't call PyLong_From* yourself, just use PyUnicode_FromFormat. Check my previous comment please.

zhangyangyu · 2019-05-05T06:15:48Z

Modules/mmapmodule.c

+#endif
+    {
+        reprfmt = "<%s closed=True access=%s>";
+        repr = PyUnicode_FromFormat(reprfmt, tp_name, access_str);


Even for closed one, I think offset and length could still be displayed, they are passed in by users and not gonna be changed. We could tell more info then.

In fact they could get changed: https://github.com/python/cpython/blob/master/Modules/mmapmodule.c#L557

Doesn't matter. Once it's closed, you can't do more operations on it. Just show what they are when they are closed.

jdemeyer · 2019-07-08T13:48:10Z

Modules/mmapmodule.c

@@ -712,6 +766,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},


Why is this being added here? tp_repr should be sufficient.

Thanks. Just got it after doing some search. Will remove this.

thautwarm added 4 commits October 14, 2018 04:41

bpo-34953: Implement mmap.mmap.__repr__

75856fb

done

718a6b0

follow PEP7

c6900ee

follow PEP7

d9d8611

the-knights-who-say-ni added the CLA not signed label Oct 15, 2018

bedevere-bot added the awaiting review label Oct 15, 2018

thautwarm changed the title ~~bpo-34953~~ bpo-34953 Implement mmap.mmap.__repr__ Oct 15, 2018

zhangyangyu changed the title ~~bpo-34953 Implement mmap.mmap.__repr__~~ bpo-34953: Implement mmap.mmap.__repr__ Oct 16, 2018

zhangyangyu self-requested a review October 16, 2018 02:32

zhangyangyu added the skip news label Oct 16, 2018

zhangyangyu reviewed Oct 16, 2018

View reviewed changes

the-knights-who-say-ni added CLA signed and removed CLA not signed labels Oct 16, 2018

thautwarm added 4 commits April 19, 2019 11:08

remove redundant line wraps and compat MS_WINDOWS

8f1546a

fix accessing data from m_obj

9d87c5c

add tests

9c77de3

compat windows and add exhaustive tests

3226a35

zhangyangyu reviewed Apr 26, 2019

View reviewed changes

Lib/test/test_mmap.py Outdated Show resolved Hide resolved

thautwarm added 2 commits April 26, 2019 21:11

follow Zhang's guides to refine codes

e4c2440

fix tests

ff18f7d

avoid showing entire_contents according to core devs' points of view

3b1d544

zhangyangyu reviewed Apr 29, 2019

View reviewed changes

thautwarm added 2 commits April 29, 2019 18:01

test data with long int size & refine repr format args & remove redun…

d5ffb47

…dant codes

invalid access now causes Py_UNREACHABLE

09b9554

zhangyangyu reviewed Apr 29, 2019

View reviewed changes

remove 'fileno' due to code review and discussion & refine wording(cl…

a1b589f

…arify usage of 'pos', 'offset' and 'length')

zhangyangyu reviewed May 5, 2019

View reviewed changes

jdemeyer requested changes Jul 8, 2019

View reviewed changes

bedevere-bot added awaiting core review and removed awaiting review labels Jul 8, 2019

zhangyangyu added 3 commits October 13, 2019 16:31

Merge branch 'master' into bpo-34953

e5f3237

adjusts

39d84a1

replace PAGESIZE with ALLOCATIONGRANULARITY

bf68fc7

zhangyangyu requested a review from jdemeyer October 13, 2019 17:19

zhangyangyu approved these changes Oct 14, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Oct 14, 2019

zhangyangyu merged commit d8ca235 into python:master Oct 17, 2019

bedevere-bot removed the awaiting merge label Oct 17, 2019

jacobneiltaylor pushed a commit to jacobneiltaylor/cpython that referenced this pull request Dec 5, 2019

bpo-34953: Implement mmap.mmap.__repr__ (pythonGH-9891)

a4023f4

shihai1991 pushed a commit to shihai1991/cpython that referenced this pull request Jan 31, 2020

bpo-34953: Implement mmap.mmap.__repr__ (pythonGH-9891)

a1a7a7b


		reprfmt = "<%s is_closed=True fileno=%d access=%s>";
		repr = PyUnicode_FromFormat(reprfmt, tp_name, fd, access_str);

Uh oh!

bpo-34953: Implement mmap.mmap.__repr__ #9891

bpo-34953: Implement mmap.mmap.__repr__ #9891

Uh oh!

Conversation

thautwarm commented Oct 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

the-knights-who-say-ni commented Oct 15, 2018

Uh oh!

zhangyangyu Oct 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhangyangyu commented Oct 16, 2018

Uh oh!

thautwarm commented Oct 16, 2018

Uh oh!

csabella commented Apr 10, 2019

Uh oh!

thautwarm commented Apr 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thautwarm commented Apr 19, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thautwarm commented Apr 26, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhangyangyu Apr 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhangyangyu Apr 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhangyangyu Apr 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhangyangyu May 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bpo-34953: Implement `mmap.mmap.repr` #9891

bpo-34953: Implement `mmap.mmap.repr` #9891

thautwarm commented Oct 15, 2018 •

edited

Loading

zhangyangyu Oct 16, 2018 •

edited

Loading

thautwarm commented Apr 10, 2019 •

edited

Loading

zhangyangyu Apr 29, 2019 •

edited

Loading

zhangyangyu Apr 30, 2019 •

edited

Loading

zhangyangyu Apr 29, 2019 •

edited

Loading

zhangyangyu May 5, 2019 •

edited

Loading

zhangyangyu May 6, 2019 •

edited

Loading