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 _______________________________________________ Python-Dev mailing list Python-Dev@python.org http://mail.python.org/mailman/listinfo/python-dev Unsubscribe: http://mail.python.org/mailman/options/python-dev/archive%40mail-archive.com
