Re: [Qemu-devel] [RFC PATCH v1 03/22] target/i386: Use prefix, aflag and dflag from DisasContext

[Jan Bobek <jan.bobek@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2329 bytes --]

Hi Aleksandar,

thanks a lot for your feedback! I have to admit that I paid little
attention to this particular patch, because it was authored by
Richard; I simply included it verbatim. I agree that it would be
clearer if it were split into three patches, and the description could
be made less confusing.  I will make sure to include your suggestions
in v2.

Thanks a lot for looking over my code!

Best,
-Jan

On 7/31/19 4:04 PM, Aleksandar Markovic wrote:
> 
> 
> On Wed, Jul 31, 2019 at 9:41 PM Aleksandar Markovic <aleksandar.m.mail@gmail.com <mailto:aleksandar.m.mail@gmail.com>> wrote:
> 
> 
> 
>     On Wed, Jul 31, 2019 at 7:59 PM Jan Bobek <jan.bobek@gmail.com <mailto:jan.bobek@gmail.com>> wrote:
> 
>         From: Richard Henderson <rth@twiddle.net <mailto:rth@twiddle.net>>
> 
>         The variables are already there, we just have to hide the ones
>         in disas_insn so that we are forced to use them.
> 
>         Signed-off-by: Richard Henderson <rth@twiddle.net <mailto:rth@twiddle.net>>
>         ---
>          target/i386/translate.c | 299 ++++++++++++++++++++--------------------
>          1 file changed, 152 insertions(+), 147 deletions(-)
> 
> 
>     Hi, Jan.
> 
>     The series overall looks great, and hopefully you will refine rough
>     around the edges parts soon. Thanks for this valuable contribution!
> 
>     About this patch, I noticed that it mentions "aflag" in the title, but
>     the patch actually does not change any code related to the variable
>     "aflag" in the described sense - it looks to me it just reduces the
>     scope of the local variable "aflag", which is certainly different than
>     "use aflag from DisasContext" as it could be implied from the
>     patch title. You definitely should not confuse the readers with
>     such inaccuracies.
> 
> 
> Also, Jan, you need to correct the code alignment (indentation), if
> you enclose a part of a function to form a new code block. I guess
> you just left these cosmetic things for v2 or later.
> 
> Sincerely,
> Aleksandar
>  
> 
> 
>     Actually, I think the patch would look much better if split into three
>     patches (easier for reviewing, and also clearer for future developers),
>     wouldn't it?
> 
>     Yours,
>     Aleksandar
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]