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

[=?big5?b?RG91ZyBLd2FuICjD9q62vHcp?= <dougkwan at google dot com>]

I moved all ULEB and unaligned integer I/O functions to
int_encoding.{h,cc} and made other suggested changes.   Here is an
updated patch.  Thanks.

-Doug

2009-12-07  Doug Kwan  <dougkwan@google.com>

        * Makefile.am (CCFILES): Add attributes.cc and int_encoding.cc.
        (HFILES): Add attributes.h and int_encoding.h.
        * Makefile.in: Regenerate.
        * dwarf_reader.cc (read_unsigned_LEB_128, read_signed_LEB_128): Move
        function definitions to int_encoding.cc
        * dwarf_reader.h (read_unsigned_LEB_128, read_signed_LEB_128): Move
        prototypes to int_encoding.h
        * reduced_debug_output.cc (int_encoding.h): New include.
        (write_unsigned_LEB_128, get_length_as_unsigned_LEB_128): Move
        function definitions to int_encoding.cc
        (insert_into_vector, read_from_pointer): Move template definitions to
        int_encoding.h
        * attributes.cc: New file.
        * attributes.h: New file.
        * int_encoding.cc: New file.
        * int_encoding.h: New file.

2009/12/7 Ian Lance Taylor <iant@google.com>:
> "Doug Kwan (Ãö®¶¼w)" <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
>

Attachment: patch-attr-2.txt
Description: Text document