Re: [PATCH v2][PR gdb/24052] Implement 'set print zero-values on|off'

[Pedro Alves <palves@redhat.com>]

Hi,

I take it that false booleans are also considered
zero, and not shown.  I guess that might be reasonable, though
I could also see someone arguing about it.

Enums with zero value are more dubious, like:

 enum foo { FOO = -1, BAR = 0, QUX = 1 };

From looking at the implementation it seems to me like zero-values off
makes us not print a BAR value.  Not sure that's really desirable.

It seems more clear to me that it'd be desirable to suppress a
zero with flag enums than with regular enums, like with this:

 enum bitmask { FOO_BIT = 1, BAR_BIT = 2, QUX_BIT = 4 };

Here's where you can check whether you have a flag enum:

 /* * True if this type is a "flag" enum.  A flag enum is one where all
    the values are pairwise disjoint when "and"ed together.  This
    affects how enum values are printed.  */

 #define TYPE_FLAG_ENUM(t) (TYPE_MAIN_TYPE (t)->flag_flag_enum)

I think we should have tests for these cases.  

There's also the question of how this interacts with Python
pretty printers:

- If there's a printer for a type or typedef that makes it so that a
zero is actually printed as some string other than "0", e.g., "JackPot!",
do we really want to suppress it?

- OTOH, if a printer decides to print a non-zero value as literal "0",
do we want to show it?

Whatever we decide, I think we should document expected behavior.

Also, it would be good to test function and method pointers too.

> @@ -194,6 +194,18 @@ cp_print_value_fields (struct value *val, struct ui_file *stream,
>  	      && field_is_static (&type->field (i)))
>  	    continue;
>  
> +	  /* If requested, skip printing of zero value fields.  */
> +	  if (!options->zero_value_print
> +	      && !field_is_static (&type->field (i)))

Not sure whether you copied this by mistake from the code above, but
skipping static fields seems wrong, since there's an option
for that.  I think we should keep the the options orthogonal.

( I could now argue that this calls for a testcase making sure
that they stay orthogonal.  :-) )

> +bool
> +value_is_zero (struct value *v)
> +{
> +  struct type *type = check_typedef (value_type (v));
> +  const gdb_byte *addr = value_contents_for_printing (v);
> +  unsigned int len = TYPE_LENGTH (type);
> +  unsigned int zeros;
> +  for (zeros = 0; zeros < len; zeros++)

Write:

  for (unsigned int zeros = 0; zeros < len; zeros++)

But I have to say that I find it weird to name the iterator
variable "zeros".  Standard "i" would be better, IMO.

TYPE_LENGTH(type) is cheap, so you could also get rid of
the len variable if you'd like.

> +    {

You can drop this level of braces.

> +      if (addr[zeros] != 0)
> +	return false;
> +    }
> +  return true;
> +/* Controls printing of zero value members.  */
> +static void

There should be an empty line after the comment.

> +show_zero_value_print (struct ui_file *file, int from_tty,
> +		       struct cmd_list_element *c,
> +		       const char *value)

Finally, this needs a gdb/NEWS entry.


Thanks,
Pedro Alves