Re: [PATCH] gold: Native Client target support

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

Roland McGrath <mcgrathr@google.com> writes:

> I don't know what you mean by "pure abstract base classes".
> Do you mean a class that contains nothing but abstract virtual functions?

Yes.

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

It's probably excessive.  Still, you've got three different child
classes for the i386 and x86_64 PLT classes, some discipline doesn't
hurt.


>> 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.

I feel that putting large functions in a class definition makes it
harder to understand the class.  It's true that breaking out the
function means that the function signature has to be repeated, but I
don't see that as a major issue because you don't really have to look at
it.  You know that the compiler has ensured that it is the same.

> 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.

I suppose I disagree.  It's true that I am relatively anal about
programming style, and I think it's unfortunate that various patches
have gone into gold that don't maintain the style.  But, whether you
agree with it or not, the style was chosen thoughtfully, not
arbitrarily.  And even if it were arbitrary, there is a significant
benefit to having a consistent style.


>>> + Â Â// 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.

Whoops, you're right.  I managed to misread the code somehow.


> 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.

You can post again if you like--indeed, you should probably post the
final version of the patch--but I'll trust you to make the appropriate
changes.  You don't have to wait for me to read through the patch again.

Thanks.

Ian