>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. Okay, going though the issues Neal raised... >* remove TODO comment at top of file--it's empty Was fixed. >* is CSV going to be maintained outside the python tree? > If not, remove the 2.2 compatibility macros for: > PyDoc_STR, PyDoc_STRVAR, PyMODINIT_FUNC, etc. Does anyone thing we should continue to maintain this 2.2 compatibility? >* inline the following functions since they are used only in one place > get_string, set_string, get_nullchar_as_None, set_nullchar_as_None, > join_reset (maybe) It was done that way as I felt we would be adding more getters and setters to the dialect object in future. >* 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? >* is it necessary to have Dialect_methods, can you use 0 for tp_methods? I was assuming I would need to add methods at some point (in fact, I did have methods, but removed them). >* remove commented out code (PyMem_DEL) on line 261 > Have you used valgrind on the test to find memory overwrites/leaks? No, valgrind wasn't used. >* PyString_AsString()[0] on line 331 could return NULL in which case > you are dereferencing a NULL pointer Was fixed. >* note sure why there are casts on 0 pointers > lines 383-393, 733-743, 1144-1154, 1164-1165 To make it easier when the time comes to add one of those members. >* Reader_getiter() can be removed and use PyObject_SelfIter() Okay, wasn't aware of PyObject_SelfIter - will fix. >* 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). >* 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)? >* iteratable should be iterable? (line 1088) Sorry, I don't know what you're getting at here? (now line 1162). >* why doesn't csv_writerows() have a docstring? csv_writerow does Was fixed. >* any PyUnicode_* methods should be protected with #ifdef Py_USING_UNICODE Was fixed. >* csv_unregister_dialect, csv_get_dialect could use METH_O > so you don't need to use PyArg_ParseTuple Was fixed. >* in init_csv, recommend using > PyModule_AddIntConstant and PyModule_AddStringConstant > where appropriate Was fixed. -- Andrew McNamara, Senior Developer, Object Craft http://www.object-craft.com.au/ _______________________________________________ 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
