Message36775
Logged In: YES
user_id=89016
> * please don't place more than one C statement on one line
> like in:
> """
> + unicode = unicode2; unicodepos =
> unicode2pos;
> + unicode2 = NULL; unicode2pos = 0;
> """
OK, done!
> * Comments should start with a capital letter and be
> prepended
> to the section they apply to
Fixed!
> * There should be spaces between arguments in compares
> (a == b) not (a==b)
Fixed!
> * Where does the name "...Encode121" originate ?
encode one-to-one, it implements both ASCII and latin-1
encoding.
> * module internal APIs should use lower case names (you
> converted some of these to PyUnicode_...() -- this is
> normally reserved for APIs which are either marked as
> potential candidates for the public API or are very
> prominent in the code)
Which ones? I introduced a new function for every old one,
that had a "const char *errors" argument, and a few new ones
in codecs.h, of those PyCodec_EncodeHandlerForObject is
vital, because it is used to map for old string arguments to
the new function objects. PyCodec_RaiseEncodeErrors can be
used in the encoder implementation to raise an encode error,
but it could be made static in unicodeobject.h so only those
encoders implemented there have access to it.
> One thing which I don't like about your API change is that
> you removed the Py_UNICODE*data, int size style arguments > --
> this makes it impossible to use the new APIs on non-Python
> data or data which is not available as Unicode object.
I look through the code and found no situation where the
Py_UNICODE*/int version is really used and having two
(PyObject *)s (the original and the replacement string),
instead of UNICODE*/int and PyObject * made the
implementation a little easier, but I can fix that.
> Please separate the errors.c patch from this patch -- it
> seems totally unrelated to Unicode.
PyCodec_RaiseEncodeErrors uses this the have a \Uxxxx with
four hex digits. I removed it.
I'll upload a revised patch as soon as it's done.
|
|
Date |
User |
Action |
Args |
2007-08-23 15:06:03 | admin | link | issue432401 messages |
2007-08-23 15:06:03 | admin | create | |
|