Re: [PATCH 1/2][PR gdb/20239] Make evaluation and type-printing of all NonZero-optimized enums work

[Tom Tromey <tom@tromey.com>]

>>>>> "Manish" == Manish Goregaokar <manish@mozilla.com> writes:

Manish> Regarding the xstrdup, this is done because strsep edits the string
Manish> itself.

Indeed it does.  Somehow I didn't realize this, sorry about that.

Manish> Methods tests fail, but they are
Manish> a preexisting failure due to Rust's debuginfo changing.

Yes, the situation with 1.9 is not that good :(


This patch looks good.  There are formatting nits but really nothing serious.

The normal procedure is that once you've written one good patch you can
get write-after-approval access to gdb.  If you want to go this route,
let me know.  Otherwise (and also let me know) I can push the final
patches for you.

Manish> +         char *type_name;

I think this is assigned to but not used, so you might as well remove
it.

Manish> +      /* Optimized enums have only one field */

gdb style is period with two spaces at the end of a comment.

Manish> +      while ((token = strsep (&tail, "$")) != NULL)
Manish> +      {

The "{" should be indented 2 more spaces, and then the body as well.

Manish> +         if (sscanf (token, "%lu", &fieldno) != 1)
Manish> +         {

Here too.

Manish> +            /* We have reached the enum name, which cannot start with a digit */

The period thing again.  Also this line might be too long.

Manish> +      };

Extra ";".

Manish> +        value = unpack_long (member_type,
Manish> +           valaddr + embedded_offset);

Make sure the continuation line is indented to line up under "member_type".
(Maybe it is but I can't tell ...)

Manish>      int i, len = 0;
Manish> +  int skip_to = 1; /* Skip the discriminant field */

Indentation?  Also gdb doesn't tend to use trailing comments like that;
you can put it before the declaration.

Manish> +    char *zero_field = strrchr (TYPE_FIELD_NAME (type, 0), '$');

const char *

Manish> +    if (zero_field != NULL && strlen (zero_field) > 1) {

"{" on the next line and indented.

Manish> +      fprintfi_filtered (level+2, stream, "%s,\n", zero_field+1);

Spaces around "+"s.

Manish> +      /* there is no explicit discriminant field, skip nothing */

Upper case "There", period.

Manish> -        int first = 1;
Manish> +    int first = 1;

Some spurious change maybe?

Tom