diff mbox

[4/7] x86emul: fold SrcImmByte fetching

Message ID 57AC861F02000078001050D2@prv-mh.provo.novell.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jan Beulich Aug. 11, 2016, 12:05 p.m. UTC 
There's no need for having identical code spelled out twice.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
x86emul: fold SrcImmByte fetching

There's no need for having identical code spelled out twice.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -1979,9 +1979,12 @@ x86_emulate(
             goto done;
         break;
     case SrcImm:
+        if ( !(d & ByteOp) )
+            src.bytes = op_bytes != 8 ? op_bytes : 4;
+        else
+    case SrcImmByte:
+            src.bytes = 1;
         src.type  = OP_IMM;
-        src.bytes = (d & ByteOp) ? 1 : op_bytes;
-        if ( src.bytes == 8 ) src.bytes = 4;
         /* NB. Immediates are sign-extended as necessary. */
         switch ( src.bytes )
         {
@@ -1990,11 +1993,6 @@ x86_emulate(
         case 4: src.val = insn_fetch_type(int32_t); break;
         }
         break;
-    case SrcImmByte:
-        src.type  = OP_IMM;
-        src.bytes = 1;
-        src.val   = insn_fetch_type(int8_t);
-        break;
     }
 
     /* Decode and fetch the destination operand: register or memory. */

Comments

Andrew Cooper Aug. 11, 2016, 1:41 p.m. UTC | #1 
On 11/08/16 13:05, Jan Beulich wrote:
> There's no need for having identical code spelled out twice.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -1979,9 +1979,12 @@ x86_emulate(
>              goto done;
>          break;
>      case SrcImm:
> +        if ( !(d & ByteOp) )
> +            src.bytes = op_bytes != 8 ? op_bytes : 4;
> +        else

{

> +    case SrcImmByte:
> +            src.bytes = 1;

}

Otherwise, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Jan Beulich Aug. 11, 2016, 2:09 p.m. UTC | #2 
>>> On 11.08.16 at 15:41, <andrew.cooper3@citrix.com> wrote:
> On 11/08/16 13:05, Jan Beulich wrote:
>> There's no need for having identical code spelled out twice.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
>> @@ -1979,9 +1979,12 @@ x86_emulate(
>>              goto done;
>>          break;
>>      case SrcImm:
>> +        if ( !(d & ByteOp) )
>> +            src.bytes = op_bytes != 8 ? op_bytes : 4;
>> +        else
> 
> {
> 
>> +    case SrcImmByte:
>> +            src.bytes = 1;
> 
> }

I don't understand: This is a single statement, which we don't require
to be surrounded by braces.

Jan
Andrew Cooper Aug. 11, 2016, 3:06 p.m. UTC | #3 
On 11/08/16 15:09, Jan Beulich wrote:
>>>> On 11.08.16 at 15:41, <andrew.cooper3@citrix.com> wrote:
>> On 11/08/16 13:05, Jan Beulich wrote:
>>> There's no need for having identical code spelled out twice.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>
>>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
>>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
>>> @@ -1979,9 +1979,12 @@ x86_emulate(
>>>              goto done;
>>>          break;
>>>      case SrcImm:
>>> +        if ( !(d & ByteOp) )
>>> +            src.bytes = op_bytes != 8 ? op_bytes : 4;
>>> +        else
>> {
>>
>>> +    case SrcImmByte:
>>> +            src.bytes = 1;
>> }
> I don't understand: This is a single statement, which we don't require
> to be surrounded by braces.

Yes it is syntactically correct, but it is very deceptive as the case
statement obscures the nature of the else scope.

~Andrew
diff mbox

Patch

--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -1979,9 +1979,12 @@  x86_emulate(
             goto done;
         break;
     case SrcImm:
+        if ( !(d & ByteOp) )
+            src.bytes = op_bytes != 8 ? op_bytes : 4;
+        else
+    case SrcImmByte:
+            src.bytes = 1;
         src.type  = OP_IMM;
-        src.bytes = (d & ByteOp) ? 1 : op_bytes;
-        if ( src.bytes == 8 ) src.bytes = 4;
         /* NB. Immediates are sign-extended as necessary. */
         switch ( src.bytes )
         {
@@ -1990,11 +1993,6 @@  x86_emulate(
         case 4: src.val = insn_fetch_type(int32_t); break;
         }
         break;
-    case SrcImmByte:
-        src.type  = OP_IMM;
-        src.bytes = 1;
-        src.val   = insn_fetch_type(int8_t);
-        break;
     }
 
     /* Decode and fetch the destination operand: register or memory. */