Re: [RFC - GDB Python API] New gdb.Architecture class

[Tom Tromey <tromey@redhat.com>]

>>>>> "Siva" == Siva Chandra <sivachandra@google.com> writes:

Siva> http://sourceware.org/ml/gdb-patches/2013-01/msg00296.html, the
Siva> attached patch adds a new class 'gdb.Architecture' with a single
Siva> method 'name'. I will add tests and docs after we are OK with the code
Siva> changes.

I think the basic idea is fine.
I found a few nits in the patch, nothing too serious.

Siva> +struct gdbarch *
Siva> +arch_object_to_gdbarch (PyObject *obj)
Siva> +{
Siva> +  arch_object *py_arch = (arch_object *) obj;
Siva> +  return py_arch->gdbarch;

Missing newline after declaration.

Siva> +PyObject *
Siva> +gdbarch_to_arch_object (struct gdbarch *gdbarch)
Siva> +{
Siva> +  arch_object *obj = PyObject_New (arch_object, &arch_object_type);
Siva> +  if (obj == NULL)
Siva> +    {
Siva> +      PyErr_SetString (PyExc_MemoryError,
Siva> +                       _("Could not allocate architectire object"));
Siva> +      return NULL;
Siva> +    }
Siva> +
Siva> +  obj->gdbarch = gdbarch;
Siva> +  return (PyObject *) obj;
Siva> +}

Likewise.

Also, I think there's no need to call PyErr_SetString here.
Just return NULL; PyObject_New will have already set the error
appropriately.

One other question is whether we care about object identity.
gdbarch_to_arch_object returns a new object each time.
Either way is fine depending on circumstances -- we do both already in
gdb -- but it ought to be an explicit choice.

Siva> +static PyObject *
Siva> +archpy_name (PyObject *self, PyObject *args) {

Wrong brace placement.

Siva> +  PyObject *py_name = Py_BuildValue ("s", name);
Siva> +
Siva> +  return py_name;

Just use PyString_FromString here.

Siva> +PyObject *
Siva> +gdbpy_current_arch (PyObject *self, PyObject *args) {

Wrong brace placement.

Do we need this function?  The "current arch" is kind of a problematic
feature.  It is semi-exposed to Python right now, but I think we're
mostly agreed that this was an early mistake.

I think the more desirable approach would be to have arch methods on
the appropriate objects -- Frame, but also whatever else, say maybe
objfile or inferior (I'm not really all that sure...).

Anyway, if you do need it:

Siva> +  struct gdbarch *curr_arch = get_current_arch ();

.. then why get_current_arch and not 'python_gdbarch'?

Siva> +static PyObject *
Siva> +frapy_arch (PyObject *self, PyObject *args)
Siva> +{
Siva> +  struct frame_info *frame = NULL;    /* Initialize to appease gcc warning.  */
Siva> +  frame_object *obj = (frame_object *) self;
Siva> +  volatile struct gdb_exception except;
Siva> +  enum unwind_stop_reason stop_reason;

I didn't see a use of stop_reason.

Siva> +extern PyTypeObject arch_object_type;
 
This didn't seem to be used outside of the new file, so it could just be
static there.

Tom