From: Nick Clifton <nickc@redhat.com>
To: Inderpreet Singh Baweja <Inderpreet.Baweja@noida.hcltech.com>
Cc: binutils@sources.redhat.com, "Naveen Sharma,
Noida" <naveen.sharma@noida.hcltech.com>
Subject: Re: [PATCH]: binutils patch for maxq target.
Date: Wed, 05 Jan 2005 09:48:00 -0000 [thread overview]
Message-ID: <41DBB9B1.9000501@redhat.com> (raw)
In-Reply-To: <33BC33A9E76474479B76AD0DE8A169728DF1@exch-ntd.nec.noida.hcltech.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
next prev parent reply other threads:[~2005-01-05 9:48 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2004-12-28 14:09 Inderpreet Singh Baweja
2005-01-05 9:48 ` Nick Clifton [this message]
2005-01-06 9:58 Inderpreet Singh Baweja
2005-01-10 16:36 ` Nick Clifton
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=41DBB9B1.9000501@redhat.com \
--to=nickc@redhat.com \
--cc=Inderpreet.Baweja@noida.hcltech.com \
--cc=binutils@sources.redhat.com \
--cc=naveen.sharma@noida.hcltech.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).