This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: RFA: Blackfin Binutils Port Submission
- From: Nick Clifton <nickc at redhat dot com>
- To: Catherine Moore <clm at cm00re dot com>
- Cc: binutils at sourceware dot org
- Date: Thu, 29 Sep 2005 14:32:59 +0100
- Subject: Re: RFA: Blackfin Binutils Port Submission
- References: <200509072011.j87KBRDQ004104@mx1.redhat.com>
Hi Catherine,
Please let me know if it's okay to commit.
It is. (Very sorry for taking so long to review this patch).
There are a couple of points however:
* config.sub (bfin): New.
* configure.in (bfin-*-*): New.
Changes to the toplevel configure.in file should be approved by the gdb
and gcc projects as well, and then applied to both the sourceware
version and the gcc version. Changes to the config.sub file need to be
submitted to the config project <config-patches@gnu.org> for approval.
* Makefile.in: Regenerated.
* aclocal.m4: Regenerated.
There is no need to include regenerated files in the actual patch
itself. It just takes up needless room.
---
I also have a few minor comments about pieces of the patch itself:
> bfd/reloc.c
> + ENUMDOC
> + ADI Blackfin 16 bit immediate absolute reloc
Please could you add a full stop to the end of comments. It helps to
make the generated comment slightly more conformant to the GNU Coding
standard.
> + /* ADI Blackfin 16 bit immediate absolute reloc */
---
Also in a couple of places you have an unnecessary capitalized second
letter in a comment, eg:
> + ADI Blackfin 16 bit immediate absolute reloc HIgher 16 bits
This should really be:
+ ADI Blackfin 16 bit immediate absolute reloc Higher 16 bits
or maybe even:
+ ADI Blackfin 16 bit immediate absolute reloc higher 16 bits
unless the word "Higher" has particular significance to the Blackfin port ?
---
> bfd/cpu-bfin.c
> + ADI Blackfin 16 bit immediate absolute reloc HIgher 16 bits
Please could the comments in this file be converted to conform the GNU
Coding Standard ? ie with a capitial first letter, a terminating full
stop and a couple of spaces before the closing */.
In fact as a general point, all of the comments in your patch should be
checked to make sure that they conform to the standard.
---
> bfd/elf32-bfin.c+ static bfd_reloc_status_type bfin_bfd_reloc
> + PARAMS ((bfd *, arelent *, asymbol *, PTR, asection *, bfd *,
char **));
Since we are now only coding for ISO C90 conformance there is no need to
use the PARAMS macro, and in fact its use is discouraged. In fact most
of the static function prototypes in this file can be removed providing
that you reorganise the code so that a static function is defined before
it is used.
---
> gas/config/bfin-aux.h
> + /* bfin-aux.h ADI Blackfin Header file for gas^M
It appears that this file contains extraneous carriage returns at the
end of every line. (Explicitly shown above as ^M).
Cheers
Nick