On 08/29/2011 07:41 PM, Pedro Alves wrote:

> On Thursday 18 August 2011 16:36:46, Yao Qi wrote:
>
>> +
>> +CORE_ADDR
>> +bfd_lookup_symbol (bfd *abfd, const char *symname,
>> +                  int (*is_right_sym) (asymbol *, const char *))
>
> All the function does with the passed in SYMNAME is passing
> it down to the callback.  If the interface has been generalized to
> pass in a "match" callback, then make the symname parameter
> generic too, like:
>
>    CORE_ADDR
>    bfd_lookup_symbol (bfd *abfd,
>                    int (*is_right_sym) (asymbol *, void *),
>                    void *data);
>
> Please document the parameters.

OK, parameters are documented.

>
> I think it's best to reserve extern symbols that start with
> "bfd_" to libbfd proper.  Maybe gdb_bfd_lookup_symbol would
> be a good enough alternative.  (Or maybe propose the function
> for bfd proper, though I'd suggest fixing its interface
> to not assume 0 return is error then)
>

I agree.  Renamed bfd_* functions to gdb_bfd_*.

>> +/* Lookup the value for a specific symbol.
>> +
>> +   An expensive way to lookup the value of a single symbol for
>> +   bfd's that are only temporary anyway.  This is used by the
>> +   shared library support to find the address of the debugger
>> +   notification routine in the shared library.
>> +
>> +   The returned symbol may be in a code or data section; functions
>> +   will normally be in a code section, but may be in a data section
>> +   if this architecture uses function descriptors.
>
> The way you've generalized the function, since it's the
> callback's responsibility to pick the the symbol, this
> comment no longer appears to be in the right place.
>

OK, removed this comment here.

-- 
Yao (齐尧)