[gold][patch] Record the number of members in an archive

Ian Lance Taylor iant@google.com
Wed Dec 2 00:33:00 GMT 2009


Rafael Espindola <espindola@google.com> writes:

>        case INCREMENTAL_INPUT_ARCHIVE:
> -        printf("Archive\n");
> +        {
> +          printf("Archive\n");
> +          const Incremental_inputs_archive_header_data* archive_header =
> +            reinterpret_cast<const Incremental_inputs_archive_header_data*>
> +            (((const unsigned char *)incremental_header) + input->data_offset);
> +          printf("    Members count: %d\n", archive_header->member_count);
> +        }

Whoops, I missed this in the incremental-dump.cc patch.  This is
wrong.  You should not use the _data structs directly.  You should
only use them via an accessor class like
Incremental_inputs_archive_header.  This code will only work if the
object was created on a system with the same endianness, which is not
a desirable restriction.


> +  elfcpp::Elf_Word
> +  get_reserved(elfcpp::Elf_Word v)
> +  { return Convert<32, big_endian>::convert_host(this->p_->reserved); }

I guess I wouldn't bother with this or with put_reserved.  They will
never be used--if we define a value for that word, we will have to
change these.


> +template<int size, bool big_endian>
> +class Incremental_inputs_write
> +{
> + public:
> +  Incremental_inputs_write() : archive_count_(0), archive_member_count_(0),
> +                               object_count_(0), script_count_(0),
> +                               current_archive_offset_(0), buffer_(NULL)
> +  { }

Put the : and the members on the next line, as in other inline
constructors.

> +  void
> +  add_script()
> +  {
> +    this->script_count_++;
> +  }

Write the whole body on a single line, as in other single-line inline
methods.

> +  size_t
> +  get_size()
> +  {
> +    size_t archive_size = this->archive_count_ * this->archive_header_size_ +
> +        this->archive_member_count_ * this->archive_member_size_;

Use parentheses to get proper indentation from emacs.  Put the
operator at the start of the line, not the end.

> +  unsigned char*
> +  get_buffer()
> +  {
> +    return this->buffer_;
> +  }

Shouldn't this return "const unsigned char *"?

> +  unsigned char*
> +  get_archive_buffer(size_t offset)
> +  {
> +    return this->buffer_ + offset;
> +  }

What's the point of this?

> + private:
> +  size_t get_common_size()
> +  {

Newline after size_t.

> +    size_t file_count = this->script_count_ + this->archive_count_ +
> +      this->object_count_ + this->archive_member_count_;

Use parentheses, put operator at start of next line.



> +
> +  size_t archive_count_;
> +  size_t archive_member_count_;
> +  size_t object_count_;
> +  size_t script_count_;

Put private members last in the class definition.  Add a comment for
each member.

> +  /* Mesured from the first archive, not from the start of the section. */

ps/Mesured/Measured/.

> +  static const int entry_size_ =
> +      Incremental_inputs_entry_write<size, big_endian>::data_size;
> +  static const int header_size_ =
> +      Incremental_inputs_header_write<size, big_endian>::data_size;
> +  static const int archive_header_size_ =
> +      Incremental_inputs_archive_header_write<size, big_endian>::data_size;
> +  static const int archive_member_size_ =
> +      Incremental_inputs_archive_member_write<size, big_endian>::data_size;

These should be before the members.


In general the Incremental_inputs_write class seems very thin at the
moment.



> +        case INCREMENTAL_INPUT_ARCHIVE:
> +          {
> +            Archive_info archive = it->second.archive;
> +            size_t archive_offset =
> +              writer.get_current_archive_offset(archive.member_count);
> +            entry.put_data_offset(archive_offset);
> +            Incremental_inputs_archive_header_write<size, big_endian>
> +                archive_header = writer.get_archive_writer(archive_offset);
> +            archive_header.put_member_count(archive.member_count);
> +          }
> +          break;

Why not have a method which takes an archive index and an archive
offset and fills in the value?

> @@ -338,7 +361,7 @@ class Incremental_inputs
>      union
>      {
>        // Present if type == INCREMENTAL_INPUT_ARCHIVE.
> -      Archive* archive;
> +      struct Archive_info archive;

This is C++, no struct keyword required.

Please adjust and resend.

You can go ahead and commit the typo fixes if you want, those are
obvious.

Ian



More information about the Binutils mailing list