This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [patch] Add bfd_target_info function to bfd and unify windres/windmc target information gathering
- From: Nick Clifton <nickc at redhat dot com>
- To: Kai Tietz <ktietz70 at googlemail dot com>
- Cc: binutils at sourceware dot org
- Date: Mon, 09 Nov 2009 10:23:01 +0000
- Subject: 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