Re: [PATCH] gold: Native Client target support

[Roland McGrath <mcgrathr at google dot com>]

On Tue, May 1, 2012 at 4:03 PM, Ian Lance Taylor <iant@google.com> wrote:
> I suppose I disagree. [...]

Fair enough.  While we're on the subject, a few more style questions.

1. Do you have a tabs vs spaces policy?
   The gold code seems to differ from the rest of binutils, where
   the norm is to use tabs for all leading groups of 8 spaces, as
   is traditional GNU style.

2. What's the thinking behind the explicit this-> requirement?
   I know it's sometimes required in template code for reasons nobody
   should be required to think about.  Is it just for consistency with
   those cases, or is it that you want instance variable accesses all to
   be extremely obviously such (and the trailing_ convention is somehow
   not sufficient to distinguish them from variables) and all method calls
   to be obviously distinguished from normal function calls?

3. What about the unnecessary "virtual"?  In, out, or you don't care?
   (If you don't care, I'll leave them all in, as I find that a useful
   aid to readability.)

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

Maybe things didn't line up right when offset by the diff + prefix,
because I used tabs.

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

You may be trusting me more than I trust myself.  ;-)
But I'll try to read through it all extra carefully to save you the time.

However, I've discovered that I need to make at least one more substantive
(though minor) change in the target-selection stuff.  (I did that a while
after all the rest of the work, and it appears I didn't repeat all the
testing I'd thought I had.)  I'm adding an off_t argument after Input_file*
to select_target and Target_selector::{do_,}recognize, which is necessary
for the Target_selector_nacl logic to apply correctly to files inside
archives.


Thanks,
Roland