Re: [PATCH] Add plugin interface to LD [2/4] Claim files and add symbols.

[Dave Korn <dave dot korn dot cygwin at gmail dot com>]

On 06/10/2010 23:14, Richard Henderson wrote:

  Re:

>> +  asym->section = ldsym->def == LDPK_COMMON
>> +	? bfd_com_section_ptr
>> +	: (ldsym->def == LDPK_UNDEF || ldsym->def == LDPK_WEAKUNDEF
>> +		? bfd_und_section_ptr
>> +		: bfd_get_section_by_name (abfd, ".text"));
> 
> This is pretty hard to read as-is.  Better as
> 
>   flagword flags = BSF_NO_FLAGS;
>   struct bfd_section *section;
> 
>   switch (ldsym->def)
>     {
>     case LDPK_WEAKDEF:
>       flags = BSF_WEAK;
>       /* FALLTHRU */
>     case LDPK_DEF:
>       flags |= BSF_GLOBAL;
>       section = bfd_get_section_by_name (abfd, ".text");
>       break;
> 
>     case LDPK_WEAKUNDEF:
>       flags = BSF_WEAK;
>       /* FALLTHRU */
>     case LDPK_UNDEF:
>       section = bfd_und_section_ptr;
>       break;
> 
>     case LDPK_COMMON:
>       flags = BSF_GLOBAL;
>       section = bfd_com_section_ptr;
>       break;
> 
>     default:
>       return LDPS_ERR;
>     }
>   asym->flags = flags;
>   asym->section = section;

  I think you didn't intend to remove the "asym->value = ldsym->size;" for
common symbols in the original switch statement, and I think it's still the
right thing to do?  That's how I'm respinning it to start with, let me know if
I assumed wrong.

>> +      rv = asymbol_from_plugin_symbol (abfd, bfdsym, syms++);
> 
> Better I think just as syms + n.  I prefer not to increment
> induction variables at non-obvious spots inside the loop.
> In this case we can rely on the compiler producing the ++
> if it really turns out to be useful.

  Looking back at my source tree, I'd already removed the ++ and put syms++
into the for loop-increment clause, but I'm sure the compiler will optimise it
for us, and I also think it's a bit cleaner to not modify your formal
parameters if you really don't have to, so I redid it this way anyway.

    cheers,
      DaveK