On Thu, 2012-08-16 at 14:33 -0700, Roland McGrath wrote:
> > +2012-07-27 Mark Wielaard <m...@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
_______________________________________________
elfutils-devel mailing list
elfutils-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/elfutils-devel
