RE: [PATCH] Addition of new option '--rlib' in linker for Renesas SH targets

["Gina Verlekar" <GinaV at KPITCummins dot com>]

Hi Nick,

Thank you very much for the detailed review. 
I will implement all the mentioned changes and re-post the patch.

Regards,
Gina Verlekar
KPIT Cummins InfoSystems Ltd.
Pune, India
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~	
Free download of GNU based tool-chains for Renesas SH, H8, R8C, M16C and
M32C Series.
The following site also offers free technical support to its users.	
Visit http://www.kpitgnutools.com for details.
Latest versions of KPIT GNU tools were released on June 1, 2006.
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~	


-----Original Message-----
From: Nick Clifton [mailto:nickc@redhat.com] 
Sent: Wednesday, July 12, 2006 11:39 PM
To: Gina Verlekar
Cc: binutils@sourceware.org
Subject: Re: [PATCH] Addition of new option '--rlib' in linker for
Renesas SH targets

Hi Gina,

> Please refer to the link below for the patch I had submitted:
> http://sources.redhat.com/ml/binutils/2006-07/msg00068.html

Sorry for the delay in reviewing this patch.

Unfortunately I do not think that it is ready for inclusion into the
sources yet.  The problems that I have with it are:

  * The changes to Makefile.am mean that recreating Makefile.in
    results a warning message about the convrenesaslib_SOURCES being
    defined but not used.  You need to add it to the EXTRA_PROGRAMS
    definition.

  * The copyright comment at the start of convrenesaslib.c has the
    wrong address for the FSF and it says that the tool is part of GLD
    whereas in fact it is part of the GNU Binutils.

  * REPORT_BUGS_TO is defined in the bin-bugs.h header.

  * Having the "magic_num" field of the rlib_header structure be of
    the "unsigned char" type results in compile time warning messages
    when it is used as an argument to strcpy().

  * None of the textual output of the program has been
    internationalized.

  * The --remove-underscore switch is a bad idea.  It would be much
    cleaner and safer if you just invoke "objcopy
    --remove-leading-char" for yourself as a separate step after
    completing the conversion to the GNU ar format.

  * The indentation and formatting do not quite match up to the GNU
    Coding Standard.

  * The use of the program_name array looks very strange to me.  I
    think that it would be much cleaner if you left program_name as
    being the value of argv[0] and you used a different variable when
    you are constructing the ar command line.

  * There are various "magic" constants in some of the malloc() calls
    (eg + 50, or + 100) without any explanation of where they come
    from.  There are also several fixed length arrays that should not
    be fixed (eg filepath, module_name).

  * The fatal() function should be used in place of the
    printf();exit(1); combination currently used.

  * [optional] Adding a --verbose switch would be useful.  It could
    tell the user the command line it is using to invoke "ar", and
    also mention when files are being created and deleted.

  * [optional] A test case to check that the tool works would be very
    helpful.

Cheers
  Nick