Re: [PATCH 1/5] Rewrite DWARF string functions using known-dwarf macros.

[Mark Wielaard <mjw at redhat dot com>]

On Thu, 2012-08-16 at 14:33 -0700, Roland McGrath wrote:
> > +2012-07-27  Mark Wielaard  <mjw@redhat.com>
> 
> Use current dates in the log entries.  Note also that if you use 'git
> commit --amend' it preserves the old commit date, which is not helpful.
> If you pass --reset-author it resets the date.

Grin, we have been nitpicking this patch series for a month now...

> > +	* readelf.c (dwarf_tag_name): Renamed from dwarf_tag_string.
> > +	Uses new dwarf_tag_string or adds unknown_%#x.
> 
> No longer matches reality.

fixed.

> > +	(print_ops): Remove known[]. Use dwarf_locexpr_opcode_string.
> 
> Put local variable names in caps, and there is no need for pseudo-syntax
> like [], just the name is fine.  Put two spaces between sentences.

The CAPS thing looks a little weird, but ok.

> > +#define ONE_KNOWN_DW_FORM_DESC(NAME, CODE, DESC) ONE_KNOWN_DW_FORM(NAME, CODE)
> 
> Space before paren in the macro use.

fixed.

> > +static const char *
> > +dwarf_locexpr_opcode_string (unsigned int code)
> > +{
> > +  static const char *const known[] =
> > +    {
> > +      /* Normally we can't affort building huge table of 64K entries,
> > +	 most of them zero, just because there are a couple defined
> > +	 values at the far end.  In case of opcodes, it's OK.  */
> > +#define ONE_KNOWN_DW_OP_DESC(NAME, CODE, DESC) ONE_KNOWN_DW_OP(NAME, CODE)
> 
> Here too.

fixed.

> > +  const char *ret = NULL;
> > +  if (likely (code < sizeof (known) / sizeof (known[0])))
> > +    ret = known[code];
> > +
> > +  return ret;
> 
> It's more natural to nix RET and just return in the if.

OK.

> > +}
> > +
> > +
> > +/* Reused by all dwarf_foo_name functions.  */
> > +static char unknown_buf[16];
> 
> lo_user+4294967295\0 is 19, so make it at least 20.

fixed.

> > +static const char *
> > +dwarf_tag_name (unsigned int tag)
> > +{
> > +  const char *result = dwarf_tag_string (tag);
> > +  if (unlikely (result == NULL))
> > +    {
> > +      if (tag >= DW_TAG_lo_user && tag <= DW_TAG_hi_user)
> > +	snprintf (unknown_buf, sizeof unknown_buf, "lo_user+%#x",
> > +		  tag - DW_TAG_lo_user);
> > +      else
> > +	snprintf (unknown_buf, sizeof unknown_buf, "??? (%#x)", tag);
> > +      result = unknown_buf;
> > +    }
> > +  return result;
> > +}
> 
> This pattern is repeated a lot, so write a subroutine:
> 
> static const char *
> string_or_unknown (const char *known,
> 		   unsigned int lo_user, unsigned int hi_user)
> 
> Then unknown_buf can be static inside that function.

Done.

> I'm not too sure about the (existing) distinction between things that
> always have the number printed as well as the symbolic form.  I'm inclined
> to say we should make readelf output more uniform: symbolic name only, and
> number only if unrecognized.  But that is a behavior change that is
> technically orthogonal to your changes here.

Yes, that is why I didn't change that.

Thanks,

Mark