Re: Python: add field access by name and standard python mapping methods to gdb.Type

[Phil Muldoon <pmuldoon@redhat.com>]

Paul Koning <paulkoning@comcast.net> writes:

> The attached proposed patch adds access to struct and enum fields by their name, just as field names can be used on gdb.Value objects.  It also adds the standard Python methods for mappings (dictionary-like objects), so that the kind of things that Python programmers would expect to work on objects that can be indexed by name indeed do work.  For example, you can iterate through the fields, or query if a field name exists.  
>
> For a change like this that adds a new feature which uses a whole pile
> of new functions, is the sort of ChangeLog entry shown below the right
> way to document the change, or should I write this in a different way?

Other than one small nit, your ChangeLog is perfectly acceptable.  I
think you should on the py-type.c line, list the functions first, then
the comment.

>  
> -/* Return a sequence of all fields.  Each field is a dictionary with
> -   some pre-defined keys.  */
> +/* Helper function to return the name of a field, as a Python object.
> */

"Or None." to document what happens if a name cannot be found.

>  static PyObject *
> -typy_fields (PyObject *self, PyObject *args)
> +field_name (struct type *type, int field)
>  {
>    PyObject *result;
> +
> +  if (TYPE_FIELD_NAME (type, field)) 
> +    result = PyString_FromString (TYPE_FIELD_NAME (type, field));
> +  else
> +    {
> +      result = Py_None;
> +      Py_INCREF (result);
> +    }
> +  return result;
> +}



> +static PyObject *
> +make_fielditem (struct type *type, int i, enum gdbpy_iter_kind kind)
> +{
> +  PyObject *item = NULL, *key = NULL, *value = NULL;
> +
> +  switch (kind)
> +    {
> +    case iter_items:
> +      key = field_name (type, i);
> +      if (key == NULL)
> +	goto fail;
> +      value = convert_field (type, i);
> +      if (value == NULL)
> +	goto fail;
> +      item = PyTuple_New (2);
> +      if (item == NULL)
> +	goto fail;
> +      PyTuple_SET_ITEM (item, 0, key);
> +      PyTuple_SET_ITEM (item, 1, value);
> +      break;
> +    case iter_keys:
> +      item = field_name (type, i);
> +      break;
> +    case iter_values:
> +      item =  convert_field (type, i);
> +      break;
> +    }
> +  return item;
> +  
> + fail:
> +  Py_XDECREF (key);
> +  Py_XDECREF (value);
> +  Py_XDECREF (item);
> +  return NULL;
> +}

Because this function returns a value that is not simple (it returns a
tuple) a brief comment on the return type and the contents would be
great. 

> +typy_getitem (PyObject *self, PyObject *key)
> +{
> +  struct type *type = ((type_object *) self)->type;
> +  char *field;
> +  int i;
> +  
> +  field = python_string_to_host_string (key);
> +  if (field == NULL)
> +    return NULL;
> +
> +  /* We want just fields of this type, not of base types, so instead of 
> +     using lookup_struct_elt_type, portions of that function are
> +     copied here.  */
> +
> +  for (;;)
> +    {
> +      CHECK_TYPEDEF (type);
> +      if (TYPE_CODE (type) != TYPE_CODE_PTR
> +	  && TYPE_CODE (type) != TYPE_CODE_REF)
> +	break;
> +      type = TYPE_TARGET_TYPE (type);
> +    }

This gives me pause, not because it is wrong, but because I wonder if
there is a possibility that this loop will never exit.  I presume it
will eventually find the base_type, just by continually walking the
TARGET_TYPE until it reaches bottom.  

Can you check how this is done in other parts of GDB (this must happen
quite often?).

> +typy_get (PyObject *self, PyObject *args)
> +{
> +  PyObject *key, *defval = Py_None, *result;
> +  
> +  if (!PyArg_UnpackTuple (args, "get", 1, 2, &key, &defval))
> +    return NULL;
> +  
> +  result = typy_getitem (self, key);
> +  if (result != NULL)
> +    return result;
> +  
> +  /* getitem returned error, clear the exception status and
> +     return the defval instead.  */
> +  PyErr_Clear ();

Given that typy_getitem can raise errors that do not relate directly to
a lookup (the string conversion call in there might indicate some
other conditions, like out of memory, for example), should we only clear
exceptions in some specific case?


> +/* Implement the "has_key" method on the type object.  */
> +static PyObject *
> +typy_has_key (PyObject *self, PyObject *args)
> +{
> +  struct type *type = ((type_object *) self)->type;
> +  char *field;
> +  int i;
> +  
> +  if (!PyArg_ParseTuple (args, "s", &field))
> +    return NULL;
> +
> +  /* We want just fields of this type, not of base types, so instead of 
> +     using lookup_struct_elt_type, portions of that function are
> +     copied here.  */
> +
> +  for (;;)
> +    {
> +      CHECK_TYPEDEF (type);
> +      if (TYPE_CODE (type) != TYPE_CODE_PTR
> +	  && TYPE_CODE (type) != TYPE_CODE_REF)
> +	break;
> +      type = TYPE_TARGET_TYPE (type);
> +    }

See above for the comment on this loop.  If GDB does not have a helper
macro to this already, you might consider just adding one.

Thanks for this patch, I think it is very useful

Phil