Re: [PATCH][GOLD] Attributes section part 2

[Ian Lance Taylor <iant at google dot com>]

"Doug Kwan (éæå)" <dougkwan@google.com> writes:

> 2009-12-06  Doug Kwan  <dougkwan@google.com>
>
>         * Makefile.am (CCFILES): Add attributes.cc.
>         (HFILES): Add attributes.h.
>         * Makefile.in: Regenerate.
>         * attributes.cc: New file.
>         * attributes.h: New file.


> +const unsigned char*
> +Attributes_codec::read_uleb128(
> +    const unsigned char* p,
> +    size_t size,
> +    uint64_t* pvalue)

We already have read_unsigned_LEB_128, declared in dwarf_reader.h.
Please use or change that one, although your name is better.


> +unsigned char*
> +Attributes_codec::write_uleb128(unsigned char* p, size_t size, uint64_t value)
> +{

Similarly we already have write_unsigned_LEB_128 in
reduced_debug_output.cc, although it is not yet declared in any .h
file.  Please unify your code with that one.


> +// Return size of VALUE encoded in ULEB128.
> +
> +size_t
> +Attributes_codec::uleb128_size(uint64_t value)
> +{

Move this to dwarf_reader.cc, I guess, or move them all to some third
file.


> +	  section_size_type section_size =
> +	    convert_to_section_size_type(Attributes_codec::read_u32(p));
> +	  p += sizeof(uint32_t);

Using sizeof is kind of a host/target confusion, although it will
always work correctly.  The size of a host value is irrelevant when
working with target data.  Just write p += 4.


> +  // String value.
> +  char* string_value_;

Unless there is some overwhelming performance reason, make this a
std::string rather than a char*.  That will simplify a bunch of your
code.

> +  // Name of this written vendor subsection.
> +  const char*
> +  name() const
> +  {
> +    return this->vendor_ == Object_attribute::OBJ_ATTR_PROC
> +	   ? parameters->target().attributes_vendor()
> +	   : "gnu";
> +  }

Use parentheses around the ?: expression to get correct indentation
automatically when using emacs.


Please send another version.

Thanks.

Ian