This is the mail archive of the 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 Nick,

2009/11/9 Nick Clifton <>:
> 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.

Well, this is maybe better. I just wanted to keep the function as easy
as possible, but you are right that possibly the use of bfd_target
vector as input argument makes sense here.

> 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.

Well, I will add here some more comments. Mainly it is present to
support internal target names, which are triplets and not tuples (as
for some wince arm targets introduced). So architecture can be found

> ?+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.
Well, is this likely? But you are right, I'll add a check here.

> ?+ ? ? ?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.
Hmm, exactly this I wanted to avoid. The architecture detection is a
bit costy, so I wanted to have this check optional.

> 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.
Hmm, about the failure return NULL I agree here. To change arguments
to be bfd_boolean * instead could be right. The idea here was to have
a tri-state, but well, possibly this isn't necessary.

> ?+ ? ? ?const char ** arches = bfd_arch_list();
> Space between bfd_arch_list and ().
> Cheers
> ?Nick


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

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