Re: [PATCH] gold: Native Client target support

[Roland McGrath <mcgrathr at google dot com>]

On Tue, May 1, 2012 at 2:35 PM, Ian Lance Taylor <iant@google.com> wrote:
> gold has a general pattern that you are not following here: public
> functions in a class are never virtual, except for pure abstract base
> classes. ?The idea is that we don't want to confuse the public interface
> that a class presents to its users with the interface that child classes
> present to the parent class. ?So I would be happier if these functions
> were not virtual, but instead called protected virtual functions
> implemented by the child classes. ?There are many existing examples in
> gold, usually a public function NAME calling a protected virtual
> function do_NAME.

These are abstract base classes (they have some "virtual ... = 0"
functions).  I don't know what you mean by "pure abstract base classes".
Do you mean a class that contains nothing but abstract virtual functions?

I'll do it however you want, but frankly this sort of discipline seems
excessive for classes that are private to one file.

> There is no reason for these large functions to be declared in the class
> itself. ?The function implementations should be moved out.

The obvious reason is that the entire class is private to one source file,
so there is no meaningful interface/implementation distinction anyway and
implementing them directly in the class avoids having to repeat the
function signatures.

It seems to me like you're applying ideas that make sense in the general
case of classes actually used modularly in multiple source files to cases
where their only practical effect is to make writing the code require more
typing and reading it require more flipping back and forth.

I'll conform to whatever style constraints you want to specify.
But these really seem like knee-jerk reactions in a context where
they are actually counterproductive for the ease of maintenance.

>> + ? ?// Calculate the displacement between the PLT slot and the
>> + ? ?// common tail that's part of the special initial PLT slot.
>> + ? ?int32_t tail_displacement = (plt_address + (11 * sizeof(uint32_t))
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?- (plt_address + plt_offset
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? + sizeof(plt_entry) + sizeof(uint32_t)));
>
> Add parentheses around the whole expression so that emacs indents
> correctly.

I'm confused.  The cited example already has parentheses around the whole
expression and indentation from C-M-q.

>> @@ -693,10 +839,12 @@ const Target::Target_info Target_i386::i386_info =
>> ? ?true, ? ? ? ? ? ? ? ? ? ? ?// is_default_stack_executable
>> ? ?true, ? ? ? ? ? ? ? ? ? ? ?// can_icf_inline_merge_sections
>> ? ?'\0', ? ? ? ? ? ? ? ? ? ? ?// wrap_char
>> - ?"/usr/lib/libc.so.1", ? ? ?// dynamic_linker
>> + ?"/lib/ld-nacl-x86-32.so.1", // dynamic_linker
>
> This change looks wrong. ?Please revert.

Oops!  Yes, I changed the wrong one.

> It looks in some cases FILE can be NULL here. ?You should probably skip
> calling this->recognize_nacl_file in that case.

Right.

>> - ?Target* do_instantiate_target()
>> + ?virtual Target*
>> + ?do_instantiate_target()
>> ? ?{ return new Target_powerpc<size, big_endian>(); }
>> ?};
>
> Thanks for correcting the formatting here. ?Note that it's not necessary
> to use virtual here, and I tend to omit it for functions that are
> intended to be final.

Hmm.  Ok.  I thought I'd noticed a pattern of using virtual on every
virtual function, but perhaps I misunderstood some of what I was seeing.
I did this change because I found it easy to overlook the overrides that
needed to be changed when I changed a signature, because I didn't see
virtual on every instance.

>> +
>> + private:
>> + ?// The size of an entry in the PLT.
>> + ?static const int plt_entry_size = 64;
>> +
>> + ?// The first entry in the PLT.
>> + ?// From the AMD64 ABI: "Unlike Intel386 ABI, this ABI uses the same
>> + ?// procedure linkage table for both programs and shared objects."
>
> I'm not sure the comment from the AMD64 ABI is relevant here.

True enough.  I had just copied it.

> This patch is OK with those changes.

Thanks very much for the feedback.  Unless my rationale above convinced
you to relax any of your directives, I'll make all the style changes you
requested.  There are enough such nits easily overlooked that I'll post
again for final sign-off before committing.


Thanks,
Roland