From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17943 invoked by alias); 5 Jan 2005 09:48:53 -0000 Mailing-List: contact binutils-help@sources.redhat.com; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: binutils-owner@sources.redhat.com Received: (qmail 17807 invoked from network); 5 Jan 2005 09:48:35 -0000 Received: from unknown (HELO mx1.redhat.com) (66.187.233.31) by sourceware.org with SMTP; 5 Jan 2005 09:48:35 -0000 Received: from int-mx1.corp.redhat.com (int-mx1.corp.redhat.com [172.16.52.254]) by mx1.redhat.com (8.12.11/8.12.11) with ESMTP id j059mU0F006921 for ; Wed, 5 Jan 2005 04:48:35 -0500 Received: from pobox.surrey.redhat.com (pobox.surrey.redhat.com [172.16.10.17]) by int-mx1.corp.redhat.com (8.11.6/8.11.6) with ESMTP id j059mTr21523; Wed, 5 Jan 2005 04:48:29 -0500 Received: from [172.31.0.98] (vpnuser2.surrey.redhat.com [172.16.9.2]) by pobox.surrey.redhat.com (8.12.8/8.12.8) with ESMTP id j059mS0a028389; Wed, 5 Jan 2005 09:48:28 GMT Message-ID: <41DBB9B1.9000501@redhat.com> Date: Wed, 05 Jan 2005 09:48:00 -0000 From: Nick Clifton User-Agent: Mozilla Thunderbird 1.0RC1 (X11/20041201) MIME-Version: 1.0 To: Inderpreet Singh Baweja CC: binutils@sources.redhat.com, "Naveen Sharma, Noida" Subject: Re: [PATCH]: binutils patch for maxq target. References: <33BC33A9E76474479B76AD0DE8A169728DF1@exch-ntd.nec.noida.hcltech.com> In-Reply-To: <33BC33A9E76474479B76AD0DE8A169728DF1@exch-ntd.nec.noida.hcltech.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit X-SW-Source: 2005-01/txt/msg00038.txt.bz2 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