[Python-Dev] Re: [Csv] csv module TODO list (original) (raw)

Neal Norwitz neal at metaslash.com
Tue Jan 11 00:31:26 CET 2005

On Wed, Jan 05, 2005 at 10:08:49PM +1100, Andrew McNamara wrote:

>Also, review comments from Neal Norwitz, 22 Mar 2003 (some of these should >already have been addressed):

I should apologise to Neal here for not replying to him at the time.

Hey, I'm impressed you got to them. :-) I completely forgot about it.

>* rather than use PyErrBadArgument, should you use assert? > (first example, Dialectsetquoting, line 218)

You mean C assert()? I don't think I'm really following you here - where would the type of the object be checked in a way the user could recover from?

IIRC, I meant C assert(). This goes back to a discussion a long time ago about what is the preferred way to handle invalid arguments. I doubt it's important to change.

>* I think you need PyErrNoMemory() before returning on line 768, 1178

The examples I looked at in the Python core didn't do this - are you sure? (now lines 832 and 1280).

Originally, they were a plain PyObject_NEW(). Now they are a PyObject_GC_New() so it seems no further change is necessary.

>* is PyStringAsString(self->dialect->lineterminator) on line 994 > guaranteed not to return NULL? If not, it could crash by > passing to memmove. >* PyStringAsString() can return NULL on line 1048 and 1063, > the result is passed to joinappend()

Looking at the PyStringAsString implementation, it looks safe (we ensure it's really a string elsewhere)?

Ok. Then it should be fine. I spot checked lineterminator and it looked ok.

>* iteratable should be iterable? (line 1088)

Sorry, I don't know what you're getting at here? (now line 1162).

Heh, I had to read that twice myself. It was a typo (assuming I wasn't completely wrong)--an extra "at", but it doesn't exist any longer.

I don't think there are any changes remaining to be done from my original code review.

BTW, I always try to run valgrind before a release, especially major releases.

Neal

More information about the Python-Dev mailing list