This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: Patch: add new score3 target and other changes
- From: Nick Clifton <nickc at redhat dot com>
- To: brain dot lin at sunplusct dot com
- Cc: binutils at sourceware dot org
- Date: Wed, 14 Jan 2009 12:34:35 +0000
- Subject: Re: Patch: add new score3 target and other changes
- References: <OF91C6464F.EB01B604-ON4825753E.00227AB3-4825753E.00227ADF@sunplusct.com>
Hi Brain,
And, here is a patch to add some changes about score target to GNU
Binutils.
Pls check them.
Sure - although there is no need to send the patch multiple times. I
may be slow, but I do try to get around to answering all my emails. If
you think I may have forgotten an email, (which does happen), you can
just send me a ping.
Anyway, here are my comments on your patch. In general the code is fine
and these comments are much more in the way of nit-picking than any real
complaints:
* You could save space in the patch file by omitting the files that
are regenerated (eg: bfd-in.h).
* Additions to the various ChangeLogs should not be specified as
patches, but rather just included as plain text. The ChangeLogs change
so frequently that a patch to them almost never applies cleanly.
* We are no longer using the PARAMS macro. (Some old part of the
binutils code still uses them, but they will be tidied up one day). In
fact we are no longer trying to remain compliant with ANSI standard C
and instead we are following the ISO C standard. So that means there is
no need to use PARAMS and function declarations include the types of
the parameters alongside the names of the parameters. Hence this:
static const bfd_arch_info_type *compatible
PARAMS ((const bfd_arch_info_type *, const bfd_arch_info_type *));
[...]
static const bfd_arch_info_type *
compatible (a,b)
const bfd_arch_info_type * a;
const bfd_arch_info_type * b;
{
should be:
static const bfd_arch_info_type *compatible
(const bfd_arch_info_type *, const bfd_arch_info_type *);
[...]
static const bfd_arch_info_type *
compatible (const bfd_arch_info_type * a,
const bfd_arch_info_type * b)
{
* Brand new file should not really have copyright dates in the past. So:
/* 32-bit ELF support for S+core.
Copyright 2006, 2007, 2008, 2009 Free Software Foundation, Inc.
should be:
/* 32-bit ELF support for S+core.
Copyright 2009 Free Software Foundation, Inc.
Also we are now releasing the binutils sources under version 3 of the
GNU General Public License, so this:
This program is free software; you can redistribute it and/or modify
it under the terms of the GNU General Public License as published by
the Free Software Foundation; either version 2 of the License, or
(at your option) any later version.
should be:
This program is free software; you can redistribute it and/or modify
it under the terms of the GNU General Public License as published by
the Free Software Foundation; either version 3 of the License, or
(at your option) any later version.
* There should be no need to define prototypes for globally visible
functions in a C source file. So for example this:
void
s7_bfd_score_elf_hide_symbol (struct bfd_link_info *info,
struct elf_link_hash_entry *entry,
bfd_boolean force_local);
is either redundant - if the function really is meant to be globally
visible, in which case its prototype should be in a C header file not a
C source file - or wrong - if the function is not meant to be exported,
in which case it should be declared static and a prototype is only
necessary if it is impossible to define the function before it is used.
* Code suppressed by #if 0 ... #endif should either not be included
in the patch, or else a comment should be supplied explaining why the
code is suppressed.
* Please do not use C++ style comments inside C code. eg:
// unsigned long long given,given_h , given_l;
In fact since you are commenting out these declarations, they either
should either just be removed from the sources or else a comment added
explaining why they are not being used.
* When you add new assembler pseudo-ops you should include
documentation for them in the relevant file; in this case
gas/doc/c-score.texi. (Which it appears needs to be written... :)
* Since you are adding a new CPU variant to the binutils you really
should add some new tests to the assembler testsuite, and probably the
linker testsuite as well. Plus you should mention the support in the
gas/NEWS file.
Cheers
Nick