[Python-Dev] Re: [Csv] csv module TODO list

[Neal Norwitz]

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 PyErr_BadArgument, should you use assert?
> >        (first example, Dialect_set_quoting, 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 PyErr_NoMemory() 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 PyString_AsString(self->dialect->lineterminator) on line 994
> >        guaranteed not to return NULL?  If not, it could crash by
> >        passing to memmove.
> >* PyString_AsString() can return NULL on line 1048 and 1063, 
> >        the result is passed to join_append()
> 
> Looking at the PyString_AsString 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