[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