Re: [patch pe-coff]: Add support for IAT directory field

[Kai Tietz <Kai dot Tietz at onevision dot com>]

binutils-owner@sourceware.org wrote on 22.09.2010 07:21:41:

> On 20/09/2010 09:36, Kai Tietz wrote:
> > PING
> > 
> > Regards,
> > Kai
> 
>     Good morning Kai,
> 
> > +                    "__IAT_start__", FALSE, FALSE, TRUE);
> 
>   Mixed spaces/TABs at the start of the line.
> 
> +           pe_data
> (abfd)->pe_opthdr.DataDirectory[PE_IMPORT_ADDRESS_TABLE].VirtualAddress 
=
> 
>   Here, eight spaces after the first TAB should be a second TAB.
> 
> > +        }
> 
>   Also here at the end, eight spaces should be a tab.
> 
> > +        {
> > +          if (! strncmp (secname, ".idata\$", 7))
> > +            place = &hold[orphan_idata];
> > +          else
> > +            place = &hold[orphan_rodata];
> > +   }
> 
>   Likewise here, formatting vs. spaces and TABs.  I'd probably be 
inclined to
> just use a ternary operator here like
> 
>        else if ((s->flags & SEC_READONLY) == 0)
>     place = &hold[orphan_data];
>        else if ((s->flags & SEC_CODE) == 0)
> -   place = &hold[orphan_rodata];
> +   place = ! strncmp (secname, ".idata\$", 7)
> +      ? &hold[orphan_idata]
> +      : &hold[orphan_rodata];
>        else
>     place = &hold[orphan_text];
> 
> that but it doesn't matter.  Formatting issues aside, I have one issue 
with
> the actual code, in the way you treat results looked up in the hash 
table; for
> example, here...
> 
> > +      h1 = coff_link_hash_lookup (coff_hash_table (info),
> > +                    "__IAT_start__", FALSE, FALSE, TRUE);
> > +      if (h1 != NULL
> > +     && h1->root.u.def.section != NULL
> > +     && h1->root.u.def.section->output_section != NULL)
> > +   {
> 
>   I don't think you should unconditionally assume that the type of h1 is
> necessarily a defined symbol here.  All the other code in that function
> verifies the expected type of symbol in h1->root.type, and so shouldyour 
code.
> 
>   OK once that and the formatting is fixed.
> 
>     cheers,
>       DaveK
> 
> 
> 

Good morning Dave,

Thanks for review. Ok, adjusted whitespaces and add suggested 
modifications and applied after testing.

Regards,
Kai

|  (\_/)  This is Bunny. Copy and paste Bunny
| (='.'=) into your signature to help him gain
| (")_(") world domination.