From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 2829 invoked by alias); 21 Feb 2011 09:54:50 -0000 Received: (qmail 2821 invoked by uid 22791); 21 Feb 2011 09:54:49 -0000 X-SWARE-Spam-Status: No, hits=-2.0 required=5.0 tests=AWL,BAYES_00,TW_XZ X-Spam-Check-By: sourceware.org Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 21 Feb 2011 09:54:45 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 5D1942BAB0C; Mon, 21 Feb 2011 04:54:43 -0500 (EST) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id cdnQp8wb1z80; Mon, 21 Feb 2011 04:54:43 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id D6D092BAB01; Mon, 21 Feb 2011 04:54:42 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id CC82E1459B0; Mon, 21 Feb 2011 13:54:36 +0400 (RET) Date: Mon, 21 Feb 2011 10:13:00 -0000 From: Joel Brobecker To: Mike Frysinger Cc: gdb-patches@sourceware.org Subject: Re: [PATCH v3] sim: bfin: new port Message-ID: <20110221095436.GD2600@adacore.com> References: <201011152039.08285.vapier@gentoo.org> <201012311740.06605.vapier@gentoo.org> <201102141514.33025.vapier@gentoo.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.20 (2009-06-14) Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2011-02/txt/msg00533.txt.bz2 Please, sending compressed files makes it harder to look at the file. I think you were right to do it for a file that's 4MB, but in that case, using something a little more usual like gz, zip or bzip2 would have made things easier... I had to install the xzutil on my laptop and it wasn't without its little scary moment... > changes since v3: > - stubbed all the bootroms (i'll follow up) > - add simple jtag/watchpoint device simulation I'm not a sim expert, so I can only provide cosmetic review. However, I did notice a couple of important things: The copyright headers should mention 2011, and the license should be GPL version 3. Question: Why do you have .h files whose name starts with an underscore? For instance: sim/bfin/_proc_list.h... General comment: We should really strive to follow the same coding standards in sim/ as we do in gdb/. In particular, it would be nice to have more comments, and to follow the maximum line length as possible. For instance: > typedef enum > { > c_0, c_1, c_4, c_2, c_uimm2, c_uimm3, c_imm3, c_pcrel4, > c_imm4, c_uimm4s4, c_uimm4s4d, c_uimm4, c_uimm4s2, c_negimm5s4, c_imm5, c_imm5d, c_uimm5, c_imm6, > c_imm7, c_imm7d, c_imm8, c_uimm8, c_pcrel8, c_uimm8s4, c_pcrel8s4, c_lppcrel10, c_pcrel10, For the comments - I think that's a lot of code now, and I wont' block the commit for something like that. But, to me, code documentation is an integral part of software quality... For instance, the following function has a relatively cryptic name, particularly for someone who is not familiar with the bfin architecture: > +static const char * > +fmtconst_str (const_forms_t cf, bs32 x, bu32 pc) Remove the '*' at the beginning of each line in multi-line comments: > /* If an operation would otherwise cause a positive value to overflow > * and become negative, instead, saturation limits the result to the > * maximum positive value for the size register being used. Binary operators should be at the start of the line rather than at the end: > if ((gs < 2) || /* Dregs/Pregs as source */ > (gd < 2) || /* Dregs/Pregs as dest */ > (gs == 4 && src < 4) || /* Accumulators as source */ > (gd == 4 && dst < 4 && (gs < 4)) || /* Accumulators as dest */ I am pretty sure you already know all this; it's just that it's so much code that it's hard to always be complient, particularly if the code started in something like 2005. I have no other comment. I think you know what you're doing in the simulator area, so I'm inclined to trust your judgment on the rest of the code (if you could just remember for future contributions that comments are important). If Pedro has no further comments, the meat of the patch is OK with me. -- Joel (burp - 100K lines of patch later)