Re: [PATCH]: binutils patch for maxq target.

[Nick Clifton <nickc at redhat dot com>]

Hi Inderpreet,

> I am also including a patch for the config.sub file as I require
> This to bring the binutils in sync. With the gcc port for maxq.
> This is the third time I am posting this patch. 
> I have even tried Sending mails with this patch to
> config-patches@gnu.org
> But have received no reply or even a commit verification 

Sorry - but we (binutils) cannot accept or commit the patch to the top 
level configure files.  You will have to keep trying the 
config-patches@gnu.org address.

> And can anyone please tell me what needs to be done if I want my name in
> the Maintainers list for the maxq port?

Essentially you need to demonstrate three things:

  a) A willingness to take on the responsibility
  b) Knowledge of the particular target
  c) The programming skills to produce good code that conforms to the 
GNU Coding Standard


Thanks for generating this patch.  There are several problems with it 
however:

  *) The patch fixes a problem and adds a new feature.  We really 
prefer patches to just do one thing - fix one problem, add one new 
feature, etc.  It makes hunting down problems with patches much easier 
if they just do one thing.

  *) You added an inclusion of "coff/external.h" to the file 
bfd/coff-maxq.c but you did not update the dependency description in 
bfd/Makefile.am.  Thus if someone modifies coff/external.h at a later 
date coff-maxq.o will not (automatically) be rebuilt which could lead to 
problems.

  *) There are still places where the GNU Coding Standard is not being 
followed.  In particular you should always leave a single white space 
between a control operator and the opening parenthesis of its argument 
list.  So for example this:

      switch(bfd_get_mach (abfd))

should be:

       switch (bfd_get_mach (abfd))

One section was particularly bad.  The new function maxq_end() in 
gas/config/tc-maxq.c was provided as:

+ void
+ maxq_end()
+ {
+  if(max_version==10)
+    {
+       bfd_set_arch_mach (stdoutput, bfd_arch_maxq,bfd_mach_maxq10);
+    }
+  else
+    {
+     bfd_set_arch_mach (stdoutput, bfd_arch_maxq,bfd_mach_maxq20);
+    }
+
+ }
  arelent *

This should have been:

+ void
+ maxq_end (void)
+ {
+   if (max_version == 10)
+     bfd_set_arch_mach (stdoutput, bfd_arch_maxq, bfd_mach_maxq10);
+   else
+     bfd_set_arch_mach (stdoutput, bfd_arch_maxq, bfd_mach_maxq20);
+ }
+
  arelent *

Note the extra whitespaces helping to separate out the code into various 
regions as well as the removal of the redundant curly braces.

  *) It adds a static function "compatible" to the file bfd/cpu-maxq.c, 
but this function is never used.  In addition the field "the_default" of 
the new maxq10 bfd_arch_info structure definition is set to TRUE when it 
should be FALSE.

  *) The patch to include/coff/maxq.h defines new flags for the MAXQ10 
and MAXQ20 processors.  It seems strange however that the value for 
F_MAXQ10 should be set to 0x0030 rather than just a single bit.  Is this 
deliberate ?  If so, there ought to be a comment explaining why.

  *) The change to one of the lines in opcodes/maxq-dis.c looks very 
strange:

  		    info->fprintf_func (info->stream, " #%02xh.%d",
! 					grp.src, grp.bit_no);
--- 684,690 ----
  		    info->fprintf_func (info->stream, " #%02xh.%d",
! 					(grp.src, SRC), grp.bit_no);

  What is the purpose of evaluating grp.src only to ignore its value 
and instead use the constant SRC ?

I hope that you will consider these comments and produce a revised patch.

Cheers
  Nick