From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 3868 invoked by alias); 8 Mar 2011 04:54:27 -0000 Received: (qmail 3860 invoked by uid 22791); 8 Mar 2011 04:54:26 -0000 X-SWARE-Spam-Status: No, hits=-2.1 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW X-Spam-Check-By: sourceware.org Received: from mail-yx0-f169.google.com (HELO mail-yx0-f169.google.com) (209.85.213.169) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 08 Mar 2011 04:54:21 +0000 Received: by yxt33 with SMTP id 33so2452409yxt.0 for ; Mon, 07 Mar 2011 20:54:20 -0800 (PST) Received: by 10.151.12.20 with SMTP id p20mr5543226ybi.98.1299560060091; Mon, 07 Mar 2011 20:54:20 -0800 (PST) MIME-Version: 1.0 Received: by 10.147.125.2 with HTTP; Mon, 7 Mar 2011 20:54:00 -0800 (PST) In-Reply-To: <20110308043242.GI30306@adacore.com> References: <4D6EC5ED.3070307@vmware.com> <4D7137F6.4020000@vmware.com> <4D7527EE.9020204@vmware.com> <20110308043242.GI30306@adacore.com> From: Hui Zhu Date: Tue, 08 Mar 2011 04:58:00 -0000 Message-ID: Subject: Re: [RFA] i386-tdep.c, i386_process_record, document fall-through case. To: Joel Brobecker Cc: Michael Snyder , "gdb-patches@sourceware.org" Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable 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-03/txt/msg00515.txt.bz2 And about the code: case 0xc4: /* les Gv */ case 0xc5: /* lds Gv */ if (ir.regmap[X86_RECORD_R8_REGNUM]) { ir.addr -=3D 1; goto no_support; } switch (opcode) { case 0xc4: /* les Gv */ regnum =3D X86_RECORD_ES_REGNUM; break; case 0xc5: /* lds Gv */ regnum =3D X86_RECORD_DS_REGNUM; break; I check my code didn't very clear. This part is a "/* ELSE FALL THROUGH */= ". On Tue, Mar 8, 2011 at 12:32, Joel Brobecker wrote: >> As my poor understanding of C language, break or not break are both OK >> for this part. > > I'm going to be a little extremist, and I don't really mean what > I am about to ask, but: If the author of the code does not understand > the code, and no other maintainer is able to review associated patches, > is it time to remove that code? Interesting. Please go ahead if you want. :) BTW If somebody say something wrong about his code, his code need prepare to be removed, right? I suggest you post your words to the website of gdb. That will be powerful motto for gdb club. :) Thanks, Hui > > Speaking about the patch itself, I had a look, and I think, from > what I understand, that, YES, the fallthrough is intended. IMO, > it would have been clearer to write the code as follow: > > =A0 =A0case 0xc4: =A0 =A0 =A0/* les Gv */ > =A0 =A0case 0xc5: =A0 =A0 =A0/* lds Gv */ > =A0 =A0case 0x0fb2: =A0 =A0/* lss Gv */ > =A0 =A0case 0x0fb4: =A0 =A0/* lfs Gv */ > =A0 =A0case 0x0fb5: =A0 =A0/* lgs Gv */ > =A0 =A0 =A0if ((opcode =3D=3D 0xc4 || opcode =3D=3D 0xc5) > =A0 =A0 =A0 =A0 =A0&& ir.regmap[X86_RECORD_R8_REGNUM]) > =A0 =A0 =A0 =A0{ > =A0 =A0 =A0 =A0 =A0ir.addr -=3D 1; > =A0 =A0 =A0 =A0 =A0goto no_support; > =A0 =A0 =A0 =A0} > =A0 =A0 =A0if (i386_record_modrm (&ir)) > =A0 =A0 =A0 =A0return -1; > > (thus, not requiring a fallthrough) > > So patch is approved. > > -- > Joel >