Re: Patch: add new score3 target and other changes

[Nick Clifton <nickc at redhat dot 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