On Saturday, June 25, 2011 13:32:05 steve tell wrote: > --- /dev/null > +++ b/urjtag/bindings/python/Makefile.am > > +EXTRA_DIST = setup.py chain.c pycompat23.h t_urjtag_chain.py
one space after the "="
> +build: chain.c
> + -{ $(PYTHON) setup.py build && touch build; } || { $(RM) -r build; exit
> 1;
}
i dont think that should start with a "-". how about:
set -e; \
$(PYTHON) setup.py build; \
touch build
> +clean-local:
> + -$(RM) -r build
use -rf and drop the leading "-"
> --- /dev/null
> +++ b/urjtag/bindings/python/chain.c
>
> +static PyObject *UrjtagError;
i dont know anything about python bindings, but does this mean you can only
have one instance ? what if people instantiate multiple chains ?
> +typedef struct
> +{
> + PyObject_HEAD urj_chain_t *urchain;
> +} Chain;
if the internal naming doesnt matter, i'd prefer something like "urj_py_xxx"
rather than "Chain_xx" ...
> +static PyObject *
> +Chain_cable (Chain *self, PyObject *args)
> +{
> ...
> + assert (self->urchain == g_tstptr);
generally assert()'s are bad form in a library. it should probably return an
error code instead ...
> + return Py_BuildValue (""); // None
please convert the // comments to /* comments */
> + if (urc == NULL)
> + {
> + PyErr_SetString (PyExc_RuntimeError, "null chain");
> + return NULL;
> + }
you use this construct a lot (almost all funcs?). probably better to add a
local func like:
...
static bool
py_chain_is_valid (urj_chain_t *urc)
{
if (urc)
return true;
PyErr_SetString (PyExc_RuntimeError, "null chain");
return false;
}
....
if (!py_chain_is_valid (urc))
return NULL;
...
> + if (urc->parts == NULL)
> + {
> + PyErr_SetString (PyExc_RuntimeError,
> + "detect not called on this chain");
> + return NULL;
> + }
same feedback as above for this
> --- a/urjtag/configure.ac
> +++ b/urjtag/configure.ac
>
> +AC_ARG_ENABLE([python],
> + [AS_HELP_STRING([--enable-python], [build python language bindings for
liburjtag])],
> + [BIND_PYTHON="yes"],[BIND_PYTHON="$enableval"])
the 3rd/4th args are actually inverted here. the 3rd should be using
$enableval while the 4th handles default values.
the 4th should probably be BIND_PYTHON=detect, and then the following logic
would be something like:
if test "x$BIND_PYTHON" != "xno"; then
... detect python ...
if python-is-too-old
if BIND_PYTHON = yes
AC_MSG_ERROR(...)
else
AC_MSG_WARN(...)
fi
fi
fi
-mike
signature.asc
Description: This is a digitally signed message part.
------------------------------------------------------------------------------ All of the data generated in your IT infrastructure is seriously valuable. Why? It contains a definitive record of application performance, security threats, fraudulent activity, and more. Splunk takes this data and makes sense of it. IT sense. And common sense. http://p.sf.net/sfu/splunk-d2d-c2
_______________________________________________ UrJTAG-development mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/urjtag-development
