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: RFA: Blackfin Binutils Port Submission


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


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