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

[Andrew McNamara]

>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/