Re: [PATCH v2] gdb/python: add missing handling for anonymous members of struct and union

[Tom Tromey <tromey at redhat dot com>]

>>>>> "Li" == Li Yu <raise.sail@gmail.com> writes:

Li> gdb.Type.fields() missed handling for anonymous members.
Li> This patch fix it, below are details:

Thanks.

Do you have a copyright assignment in place?  If not, you will need
one.  Contact me off-list and I will get you started.

Li> Assume that we have a c source as below:

This patch needs a test case; so I suggest converting this into the
right form for the test suite.

Li> +struct __typy_iterator_object

Don't use leading underscores for the name here.

Li> +  typy_iterator_object *iter_obj = (typy_iterator_object *) self, *child_iter_obj;

This line is too long.  Just make a second declaration for child_iter_obj.

Li> +  char *name;

Probably could be const.

Li> +  while (iter_obj->child) /* deepest anonymous member first */
Li> +  {

The braces are in the wrong place for the GNU style.
They should be in 2 more spaces; see the GNU coding standards for details.
This occurs in a couple of places.

Li> +restart:
Li> +  while (iter_obj->field >= TYPE_NFIELDS (type))
Li> +  {
Li> +    iter_obj = iter_obj->parent;
Li> +    if (!iter_obj)
Li> +      return NULL;

This seems strange.  This returns NULL without setting a Python
exception.

Also the reference counting seems odd.  I guess I don't understand why
the typy_iterator_object has references to other iterator objects,
instead of plain gdb state.  References to other objects means that you
at least need to decref them when the iterator object is destroyed.

Li> +    Py_DECREF(iter_obj->child);

Missing space before the paren.  This occurs in a few spots.

Li> +  name = TYPE_FIELD_NAME (type, iter_obj->field);
Li> +  if (!name)
Li> +    goto abort_clean;

Why is this here?  I think you would have to check to be sure that there
is never a NULL field name created by gdb; in which case this should be
some kind of internal error.

Li> +  if (name[0]) /* mostly cases */

I don't understand this comment.  I suggest just deleting it.

Li> +  /* handing for anonymous members here */

Comments in gdb should be full sentences: start with an upper-case
letter, end with a period followed by two spaces.  See GNU standards.

Li> +     goto abort_clean;;

Double semicolon.

Li> +  child_iter_obj = (typy_iterator_object*)typy_make_iter (child_pytype, iter_obj->kind);

Line too long, should be wrapped.  Also a missing space after the first ")".

I don't understand why the iterator iterates into sub-objects.  Why not
just have a flat iterator?  That is, return a field with no name whose
type is some structure, and then let the caller iterate over that type
if need be.

Tom