[RFA] i386-tdep.c, i386_process_record, document fall-through case.

[RFA] i386-tdep.c, i386_process_record, document fall-through case.

[Michael Snyder]

[-- Attachment #1: Type: text/plain, Size: 38 bytes --]

Hui, is this right?

Thanks,
Michael


[-- Attachment #2: fallthru7.txt --]
[-- Type: text/plain, Size: 627 bytes --]

2011-03-02  Michael Snyder  <msnyder@vmware.com>

	* i386-tdep.c (i386_process_record): Document fall through.

Index: i386-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/i386-tdep.c,v
retrieving revision 1.324
diff -u -p -u -p -r1.324 i386-tdep.c
--- i386-tdep.c	8 Feb 2011 14:01:47 -0000	1.324
+++ i386-tdep.c	2 Mar 2011 22:31:15 -0000
@@ -4543,6 +4543,7 @@ Do you want to stop the program?"),
 	  ir.addr -= 1;
 	  goto no_support;
 	}
+      /* ELSE FALL THROUGH */
     case 0x0fb2:    /* lss Gv */
     case 0x0fb4:    /* lfs Gv */
     case 0x0fb5:    /* lgs Gv */

Re: [RFA] i386-tdep.c, i386_process_record, document fall-through case.

[Mark Kettenis]

> Date: Wed, 02 Mar 2011 14:34:21 -0800
> From: Michael Snyder <msnyder@vmware.com>
> 
> Hui, is this right?

Michael, if it is correct, please document these with a simple

/* FALLTHROUGH */

instead of inventing your own all caps annotations.

Thanks,

Mark

> 2011-03-02  Michael Snyder  <msnyder@vmware.com>
> 
> 	* i386-tdep.c (i386_process_record): Document fall through.
> 
> Index: i386-tdep.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/i386-tdep.c,v
> retrieving revision 1.324
> diff -u -p -u -p -r1.324 i386-tdep.c
> --- i386-tdep.c	8 Feb 2011 14:01:47 -0000	1.324
> +++ i386-tdep.c	2 Mar 2011 22:31:15 -0000
> @@ -4543,6 +4543,7 @@ Do you want to stop the program?"),
>  	  ir.addr -= 1;
>  	  goto no_support;
>  	}
> +      /* ELSE FALL THROUGH */
>      case 0x0fb2:    /* lss Gv */
>      case 0x0fb4:    /* lfs Gv */
>      case 0x0fb5:    /* lgs Gv */
> 
> --------------040400010108040302090109--
> 

Re: [RFA] i386-tdep.c, i386_process_record, document fall-through case.

[Michael Snyder]

Michael Snyder wrote:
> Hui, is this right?
> 
> Thanks,
> Michael
> 

Ping?   ;-)

Re: [RFA] i386-tdep.c, i386_process_record, document fall-through case.

[Hui Zhu]

Hi Michael,

Sorry for late.

I don't know we need this patch or not.

As some people's mail, they don't like this part code.   This part
should rewrite to share code with opcode or something.
So I think in the near future, this code will be rewritten by these
guys or somebody else.

Thanks,
Hui

On Sat, Mar 5, 2011 at 03:05, Michael Snyder <msnyder@vmware.com> wrote:
> Michael Snyder wrote:
>>
>> Hui, is this right?
>>
>> Thanks,
>> Michael
>>
>
> Ping?   ;-)
>
>

Re: [RFA] i386-tdep.c, i386_process_record, document fall-through case.

[Michael Snyder]

But should there be a break there?  Or not?



Hui Zhu wrote:
> Hi Michael,
> 
> Sorry for late.
> 
> I don't know we need this patch or not.
> 
> As some people's mail, they don't like this part code.   This part
> should rewrite to share code with opcode or something.
> So I think in the near future, this code will be rewritten by these
> guys or somebody else.
> 
> Thanks,
> Hui
> 
> On Sat, Mar 5, 2011 at 03:05, Michael Snyder <msnyder@vmware.com> wrote:
>> Michael Snyder wrote:
>>> Hui, is this right?
>>>
>>> Thanks,
>>> Michael
>>>
>> Ping?   ;-)
>>
>>

Re: [RFA] i386-tdep.c, i386_process_record, document fall-through case.

[Hui Zhu]

I am not sure.

As my poor understanding of C language, break or not break are both OK
for this part.

Thanks,
Hui

On Tue, Mar 8, 2011 at 02:46, Michael Snyder <msnyder@vmware.com> wrote:
> But should there be a break there?  Or not?
>
>
>
> Hui Zhu wrote:
>>
>> Hi Michael,
>>
>> Sorry for late.
>>
>> I don't know we need this patch or not.
>>
>> As some people's mail, they don't like this part code.   This part
>> should rewrite to share code with opcode or something.
>> So I think in the near future, this code will be rewritten by these
>> guys or somebody else.
>>
>> Thanks,
>> Hui
>>
>> On Sat, Mar 5, 2011 at 03:05, Michael Snyder <msnyder@vmware.com> wrote:
>>>
>>> Michael Snyder wrote:
>>>>
>>>> Hui, is this right?
>>>>
>>>> Thanks,
>>>> Michael
>>>>
>>> Ping?   ;-)
>>>
>>>
>
>

Re: [RFA] i386-tdep.c, i386_process_record, document fall-through case.

[Joel Brobecker]

> 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?

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:

    case 0xc4:      /* les Gv */
    case 0xc5:      /* lds Gv */
    case 0x0fb2:    /* lss Gv */
    case 0x0fb4:    /* lfs Gv */
    case 0x0fb5:    /* lgs Gv */
      if ((opcode == 0xc4 || opcode == 0xc5)
          && ir.regmap[X86_RECORD_R8_REGNUM])
        {
          ir.addr -= 1;
          goto no_support;
        }
      if (i386_record_modrm (&ir))
        return -1;

(thus, not requiring a fallthrough)

So patch is approved.

-- 
Joel

Re: [RFA] i386-tdep.c, i386_process_record, document fall-through case.

[Hui Zhu]

And about the code:
    case 0xc4:    /* les Gv */
    case 0xc5:    /* lds Gv */
      if (ir.regmap[X86_RECORD_R8_REGNUM])
        {
	  ir.addr -= 1;
	  goto no_support;
	}

      switch (opcode)
	{
	case 0xc4:    /* les Gv */
	  regnum = X86_RECORD_ES_REGNUM;
	  break;
	case 0xc5:    /* lds Gv */
	  regnum = 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 <brobecker@adacore.com> 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:
>
>    case 0xc4:      /* les Gv */
>    case 0xc5:      /* lds Gv */
>    case 0x0fb2:    /* lss Gv */
>    case 0x0fb4:    /* lfs Gv */
>    case 0x0fb5:    /* lgs Gv */
>      if ((opcode == 0xc4 || opcode == 0xc5)
>          && ir.regmap[X86_RECORD_R8_REGNUM])
>        {
>          ir.addr -= 1;
>          goto no_support;
>        }
>      if (i386_record_modrm (&ir))
>        return -1;
>
> (thus, not requiring a fallthrough)
>
> So patch is approved.
>
> --
> Joel
>

Re: [RFA] i386-tdep.c, i386_process_record, document fall-through case.

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel> I'm going to be a little extremist, and I don't really mean what
Joel> I am about to ask, but: If the author of the code does not understand
Joel> the code, and no other maintainer is able to review associated patches,
Joel> is it time to remove that code?

I think the code is understandable, just not as clear as I might like.

In my case the issue here is not understandability but rather laziness:
looking up all the details is a pain, surely somebody else will look at
it sooner than I will, etc.

In sum I think that removing it is somewhat too extremist :)

Tom

Re: [RFA] i386-tdep.c, i386_process_record, document fall-through case.

[Michael Snyder]

Hui Zhu wrote:
> And about the code:
>     case 0xc4:    /* les Gv */
>     case 0xc5:    /* lds Gv */
>       if (ir.regmap[X86_RECORD_R8_REGNUM])
>         {
> 	  ir.addr -= 1;
> 	  goto no_support;
> 	}
> 
>       switch (opcode)
> 	{
> 	case 0xc4:    /* les Gv */
> 	  regnum = X86_RECORD_ES_REGNUM;
> 	  break;
> 	case 0xc5:    /* lds Gv */
> 	  regnum = X86_RECORD_DS_REGNUM;
> 	  break;
> 
> I check my code didn't very clear.  This part is a "/* ELSE FALL THROUGH */".


Thank you.  So committed.