This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [patch] Add bfd_target_info function to bfd and unify windres/windmc target information gathering


Hi Kai,

  [Sorry for the delay in reviewing this patch].

> this patch adds to bfd's targets.c an new function, which is mainly a
> move from code in windres/windmc about target information gathering.
> This function is mainly the same as bfd_find_target function, but it
> allows to query optional for endianess, underscoring-character, and
> architecture for the target, too.

Hmm, I am not sure about this patch.  I can appreciate that you want
to eliminate the duplication of some code, but I think that it could
be done in a slightly cleaner way.  In particular I think it might be
simpler to avoid having your new function call bfd_find_target(), but
instead pass to it the bfd_target vector that is returned by
bfd_find_target().  Then it can be used for any target vector.

As for the patch itself there are a few issues:


  +/* Helper function for bfd_target_info.  */

It would be nice to say what this helper function does.


  +static int

This function should return a bfd_boolean not an int.


  +  while (*arch != NULL)

Bug - the code should check for a NULL value in the arch parameter
before using it.  In theory if bfd_arch_list() returns NULL in
bfd_target_info then _bfd_find_arch_match() could be called with a
NULL arch parameter.


  +      char end_ch = (in_a ? in_a[strlen(tname)] : 0);

Space between strlen and (tname).


  +	const bfd_target * bfd_target_info (const char *target_name, bfd *abfd,
  +					    int *is_bigendian, int *underscoring,
  +					    const char **def_target_arch);

Assuming that you agree with my suggestion above then I would consider
renaming this function to something like bfd_get_target_info() and
have it return the default target architecture.

Also the is_bigendian field really ought to be a bfd_boolean * rather
than an int *.  If an invalid target is passed to this new function
then it should return NULL and in that case none of the other
returnable parameters should be used.


  +      const char ** arches = bfd_arch_list();

Space between bfd_arch_list and ().

Cheers
  Nick


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]