classification
Title: mmap and exception type
Type: behavior Stage: patch review
Components: Versions: Python 3.3
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: rhettinger Nosy List: BreamoreBoy, haypo, neologix, ocean-city, rhettinger
Priority: low Keywords: needs review, patch

Created on 2009-02-27 17:23 by ocean-city, last changed 2011-06-02 18:11 by rhettinger.

Files
File name Uploaded Description Edit
mmap_exceptions.diff neologix, 2011-06-02 10:07 review
Messages (4)
msg82849 - (view) Author: Hirokazu Yamamoto (ocean-city) * (Python committer) Date: 2009-02-27 17:23
It seems mmap module is using inappropriate exception types. For example,

	if (! (PyString_Check(v)) ) {
		PyErr_SetString(PyExc_IndexError,
				"mmap slice assignment must be a string");
		return -1;
	}

I think this should be PyExc_TypeError.

		if (self->size >= pos && count > self->size - pos) {
			PyErr_SetString(PyExc_ValueError,
					"source or destination out of range");
			return NULL;

I think this is out of range, so PyExc_IndexError.

Of course, there is the case difficult to determine which exception is
suitable. For example, if Py_ssize_t is negative value, OverflowError or
IndexError?
msg109758 - (view) Author: Mark Lawrence (BreamoreBoy) Date: 2010-07-09 16:01
Could someone please advise on the appropriate exception types for mmap.
msg137471 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2011-06-02 10:07
Here's a patch.
msg137489 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2011-06-02 18:11
It is uncomfortable changing exception types have been a published API for over a decade, so I'll leave this one open for a bit so that others can comment.

Most of the changes in the patch are correct and match their counterparts for lists and strings.  The TypeError for slice deletion was correct as-is (it matches del 'abc'[1]).  The IndexError on slice assignment may be correct as-is (I'll do more research).  The from ValueError to SystemError is questionable.  Otherwise, the patch looks to be mostly correct (I'll give it a more thorough review later).
History
Date User Action Args
2011-06-02 18:11:00rhettingersetpriority: normal -> low

nosy: + rhettinger
messages: + msg137489

assignee: rhettinger
2011-06-02 10:07:48neologixsetfiles: + mmap_exceptions.diff

versions: + Python 3.3, - Python 3.1, Python 2.7
keywords: + patch, needs review
nosy: + haypo, neologix

messages: + msg137471
stage: patch review
2010-07-09 16:01:02BreamoreBoysetnosy: + BreamoreBoy
messages: + msg109758
2009-02-27 17:23:13ocean-citycreate