[Python-checkins] r46144 - in python/trunk: Lib/struct.py Modules/_struct.c Modules/structmodule.c

[Neal Norwitz]

Note: I'm reviewing the version current as of 3 June 2006, Revision: 46613.

> Added: python/trunk/Modules/_struct.c

> +/* compatibility macros */
> +#if (PY_VERSION_HEX < 0x02050000)
> +typedef int Py_ssize_t;
> +#endif

Why is this needed?  Is this maintained for older versions of Python?
(There are a few others elsewhere in this module.)  If it is needed,
is int correct or should it be long?

> +#ifdef __powerc
> +#pragma options align=reset
> +#endif

What is this?  Is it required?  If so it should be documented.

In get_wrapped_long, there's this code:

                        x = (long)PyLong_AsUnsignedLong(wrapped);

Why not use PyLongAsLong() so you don't need to cast?  It really
required as is?  If so, a comment explaining why would be nice.

In get_wrapped_ulong(), why is x a long rather than an unsigned long
with all the casts?

> +/* Floating point helpers */
> +
> +static PyObject *
> +unpack_float(const char *p,  /* start of 4-byte string */
> +             int le)        /* true for little-endian, false for big-endian */

If you renamed le to little_endian, you could get rid of the comments
and would be more readable IMO.  (Same with unpack_double.)

> +static int
> +bp_uint(char *p, PyObject *v, const formatdef *f)
> +{
        unsigned long x;
        Py_ssize_t i;
        if (get_wrapped_ulong(v, &x) < 0)
                return -1;
        i = f->size;
        if (i != SIZEOF_LONG) {
                unsigned long maxint = 1;
                maxint <<= (unsigned long)(i * 8);
                if (x >= maxint)
                        RANGE_ERROR(x, f, 1, maxint - 1);
        }

when you say i != SIZEOF_LONG, you really mean that i < SIZEOF_LONG,
right?  Otherwise the maxint calc wouldn't work.  Same with lp_uint.

> +static PyObject *
> +lu_int(const char *p, const formatdef *f)
> +{
        long x;
        Py_ssize_t i;
        if (get_wrapped_long(v, &x) < 0)
                return -1;
        i = f->size;
        if (i != SIZEOF_LONG) {
                if ((i == 2) && (x < -32768 || x > 32767))
                        RANGE_ERROR(x, f, 0, 0xffffL);
#if (SIZEOF_LONG != 4)
                else if ((i == 4) && (x < -2147483648L || x > 2147483647L))
                        RANGE_ERROR(x, f, 0, 0xffffffffL);
#endif
#ifdef PY_STRUCT_OVERFLOW_MASKING
                else if ((i == 1) && (x < -128 || x > 127))
                        RANGE_ERROR(x, f, 0, 0xffL);
#endif
        }

*** Why not use a switch(i)?

(Now pack_internal)

> +static PyObject *
> +s_pack(PyObject *self, PyObject *args)
> +{
...
> +               } else if (e->format == 'p') {
...
> +                       n = PyString_GET_SIZE(v);
> +                       if (n > (code->repeat - 1))
> +                               n = code->repeat - 1;
> +                       if (n > 0)
> +                               memcpy(res + 1, PyString_AS_STRING(v), n);
> +                       if (n > 255)
> +                               n = 255;
> +                       *res = Py_SAFE_DOWNCAST(n, Py_ssize_t, unsigned char);

Wait, don't you want to truncate n (length of string) *before* doing
the memcpy?  If this code is wrong, there should be a test for it.

> +PyMODINIT_FUNC
> +init_struct(void)
> +{
...
        PyModule_AddIntConstant(m, "_PY_STRUCT_RANGE_CHECKING", 1);
#ifdef PY_STRUCT_OVERFLOW_MASKING
        PyModule_AddIntConstant(m, "_PY_STRUCT_OVERFLOW_MASKING", 1);
#endif

Shouldn't those be bools?

n