From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 10887 invoked by alias); 21 Feb 2011 16:53:57 -0000 Received: (qmail 10877 invoked by uid 22791); 21 Feb 2011 16:53:56 -0000 X-SWARE-Spam-Status: No, hits=-1.9 required=5.0 tests=AWL,BAYES_00,TW_XZ,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from smtp.gentoo.org (HELO smtp.gentoo.org) (140.211.166.183) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 21 Feb 2011 16:53:51 +0000 Received: from vapier.localnet (localhost [127.0.0.1]) by smtp.gentoo.org (Postfix) with ESMTP id 33FE91B40CD; Mon, 21 Feb 2011 16:53:49 +0000 (UTC) From: Mike Frysinger To: Joel Brobecker Subject: Re: [PATCH v3] sim: bfin: new port Date: Mon, 21 Feb 2011 18:27:00 -0000 User-Agent: KMail/1.13.5 (Linux/2.6.37; KDE/4.5.5; x86_64; ; ) Cc: gdb-patches@sourceware.org References: <201011152039.08285.vapier@gentoo.org> <20110221095436.GD2600@adacore.com> In-Reply-To: <20110221095436.GD2600@adacore.com> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="nextPart4867750.h2Gq5QHSgG"; protocol="application/pgp-signature"; micalg=pgp-sha1 Content-Transfer-Encoding: 7bit Message-Id: <201102211153.47019.vapier@gentoo.org> X-IsSubscribed: yes 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/msg00553.txt.bz2 --nextPart4867750.h2Gq5QHSgG Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Content-length: 2730 On Monday, February 21, 2011 04:54:36 Joel Brobecker wrote: > 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... sorry, i'm so used to xz i expect everyone to have it ;) > > changes since v3: > > - stubbed all the bootroms (i'll follow up) > > - add simple jtag/watchpoint device simulation >=20 > 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. all the files do say 2011 that i can see ... were there ones missing that y= ou=20 saw ? much of the sim project seems to still be GPL v2, so i didnt want to try an= d=20 figure out what's going on. if this code should be GPL v3, that's fine, i = can=20 relicense it. > Question: Why do you have .h files whose name starts with an underscore? > For instance: sim/bfin/_proc_list.h... it's meant to convey that the header isnt meant to be generally included by= =20 random files. they can only be included in specific instances since they'r= e=20 simple lists which need certain preprocessor directives in place before the= y=20 can be included. i'm trying to avoid bit rot related to copying & pasting = the=20 same list of numbers over and over in multiple places. > 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. it is supposed to be the GNU coding style. if it isnt, then it's just an=20 oversight. i'll go through and try to fix up missing pieces including the= =20 ones you highlighted. > For instance: > > typedef enum > > { > >=20 > > 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_imm= 5, > > c_imm5d, c_uimm5, c_imm6, c_imm7, c_imm7d, c_imm8, c_uimm8, c_pcrel8, > > c_uimm8s4, c_pcrel8s4, c_lppcrel10, c_pcrel10, >=20 > 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: this section is simply copy & pasted from the existing disassembler. at so= me=20 point i'd like to unify it. -mike --nextPart4867750.h2Gq5QHSgG Content-Type: application/pgp-signature; name=signature.asc Content-Description: This is a digitally signed message part. Content-length: 836 -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.17 (GNU/Linux) iQIcBAABAgAGBQJNYpiaAAoJEEFjO5/oN/WB3ZUP/3PG9jr2Na3dMI3JifJz3TUk 0RzfyVFwCUEtOWG/fCZ+ev6FC8Eny78tT448cfhcabYIl9lRTz/grFBTpR1QG7ix BERuwH12Lrrkwo6wg0TV9QRXbkCfsRWn0UxhlfjLDb2xb3wLJY7AsrXuinZsJWJb aR3gtg/9Qfm6mHWS1AL2xykUe+ySb2MMlykxorO0WsWwXoAWDd7vxXenVIxWfxxx HW+6pT146LuQ8Ilc4HC1c23aHuXL6+Q/OhFhk10TbJCbs63rT16MLOxBjDHG6R1n 6JJuXeNzVxWhGAfooNdKPicMoJVnIN7ytlkcPKBA9vz8NhNae8KJMHe0JuzXU2Jn aF6tc1By72KgqDK6CYNGd1w6k+DHrQ2gS+JAgF5DbuIRkpvHT3d9Y4Iq2RC0Mbhw /PlY6QMk5n1htiZdUBeWsGNTiCR/0T9sAl5HbsDQ1ej7wyWlS8iIy2IFpcpYY4/V Vrg6kL5xTdeJ9VEbVK2lEPt+fX59GYXbRF+9TAKdHedH57sSvbHdj9XIKpAex37L K6DCEBn4YhsbTrwjwYrweaxscKC99SXdVTKkHU4NN7+PbUxrFHKCStq40EfFwKyu D9fr3j18ig21oPkF3mzb3jgp+EuRtYa8WBxgxOtvfIiSmJLoxx0y/U//TFP1bjcQ xpJsEcvwdn29pWu4W9fq =66TF -----END PGP SIGNATURE----- --nextPart4867750.h2Gq5QHSgG--