classification
Title: redundant "base" field in memoryview objects
Type: behavior Stage:
Components: Interpreter Core Versions: Python 3.0
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: pitrou Nosy List: barry, georg.brandl, gvanrossum, loewis, pitrou, teoliphant
Priority: critical Keywords: patch

Created on 2008-08-15 09:37 by pitrou, last changed 2008-08-19 18:23 by pitrou. This issue is now closed.

Files
File name Uploaded Description Edit
memapi.patch pitrou, 2008-08-15 20:19
Messages (12)
msg71164 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2008-08-15 09:37
PyMemoryViewObject has a "base" field which points to the originator of
the buffer. However, this field has become redundant now that the
Py_buffer struct has received an "obj" field which also points to the
originator of the buffer (this has been done as part of the fix to #3139).

Not removing "base" would make for a confusing and error-prone API.
However, removing it is complicated by the fact that "base" is sometimes
abused to contain a tuple (for PyBUF_SHADOW type buffers, which are
neither mentioned in the PEP nor used anywhere in py3k).
msg71173 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2008-08-15 17:49
Why is this a "critical" bug?
msg71175 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2008-08-15 18:08
Because it should be fixed before 3.0 final?
msg71179 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2008-08-15 19:10
> Because it should be fixed before 3.0 final?

And why should that be done? IMO, this can still
be fixed in 3.1, or not a fixed at all - I fail to
see the true bug (apart from the minor redundancy).
msg71180 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2008-08-15 19:24
Le vendredi 15 août 2008 à 19:10 +0000, Martin v. Löwis a écrit :
> Martin v. Löwis <martin@v.loewis.de> added the comment:
> 
> > Because it should be fixed before 3.0 final?
> 
> And why should that be done? IMO, this can still
> be fixed in 3.1, or not a fixed at all - I fail to
> see the true bug (apart from the minor redundancy).

I've filed this as critical because it is a new API and, if we change
it, we'd better change it before 3.0 is released.

It is a minor redundancy but could easily lead to subtle problems
(crashes, memory leaks...) if for whatever reason, "base" and "view.obj"
start pointing to different objects.

Of course, if other people disagree, it can be bumped down to "normal".
msg71181 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2008-08-15 19:45
> I've filed this as critical because it is a new API and, if we change
> it, we'd better change it before 3.0 is released.

I don't think it is API. The structure may be defined in a public
header, but it is not intended to be used directly. Instead,
only the functions around it should be used.

To make that clear, the structure could be moved into the C file,
and PyMemoryView should become a function (or also be moved into
the C file). This should be done before the next beta, indeed
(but I won't have the time to do it)
msg71182 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2008-08-15 20:19
Ok, here is a simple patch. It:
- moves the struct definition at the end of memoryobject.h with a
comment that the definition should not be considered public
- adds two macros for accessing the underlying Py_buffer* and PyObject*,
respectively
- removes the ill-named PyMemoryView() macro (PyMemoryView_GET_BUFFER()
can be used instead)
- renames PyMemory_Check() to PyMemoryView_Check()
- renames PyMemoryView_FromMemory() to PyMemoryView_FromBuffer()

I didn't try to clean up the existing documentation comments, although I
find them difficult to follow. I also didn't change anything in the
semantics and implementation of the memoryview object.

Let me know what you think.
msg71418 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2008-08-19 13:18
I don't have time to review the patch, but based on the description, I'm
+1 on landing this before beta 3.  I'd like to get the API right for 3.0
and it will be too late to land it after the release candidates start.
msg71449 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2008-08-19 17:18
Reviewers: GvR,

Message:
Looks good. Go ahead and submit, assuming you've run full tests.

Description:
http://bugs.python.org/issue3560

Please review this at http://codereview.appspot.com/3003

Affected files:
   Include/memoryobject.h
   Modules/_json.c
   Objects/memoryobject.c
   Objects/unicodeobject.c
msg71450 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2008-08-19 17:18
PS. The PEP probably needs an update. And the docs.

http://codereview.appspot.com/3003
msg71451 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2008-08-19 17:21
On 2008/08/19 17:18:45, GvR wrote:
> PS. The PEP probably needs an update. And the docs.

And Misc/NEWS

http://codereview.appspot.com/3003
msg71458 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2008-08-19 18:23
Well, the PEP needs some love generally speaking; many things are un- or
under-specified (and I don't understand better than anyone else). As for
docs, there aren't any ("grep -ri memoryview Doc/" yields only the two
lines referencing the "memoryview" builtin): see #2619.

The patch is committed in r65862.
History
Date User Action Args
2008-08-19 18:23:47pitrousetstatus: open -> closed
resolution: accepted -> fixed
messages: + msg71458
2008-08-19 17:21:00gvanrossumsetmessages: + msg71451
2008-08-19 17:18:46gvanrossumsetmessages: + msg71450
2008-08-19 17:18:31gvanrossumsetnosy: + gvanrossum
messages: + msg71449
2008-08-19 13:18:30barrysetnosy: + barry
resolution: accepted
messages: + msg71418
2008-08-15 20:19:52pitrousetfiles: + memapi.patch
keywords: + patch
messages: + msg71182
2008-08-15 19:58:10pitrousetassignee: pitrou
2008-08-15 19:45:04loewissetmessages: + msg71181
2008-08-15 19:24:21pitrousetmessages: + msg71180
2008-08-15 19:10:36loewissetmessages: + msg71179
2008-08-15 18:08:13georg.brandlsetnosy: + georg.brandl
messages: + msg71175
2008-08-15 17:49:39loewissetnosy: + loewis
messages: + msg71173
2008-08-15 09:37:06pitroucreate