[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC PATCH v1 03/22] target/i386: Use prefix, aflag and
From: |
Jan Bobek |
Subject: |
Re: [Qemu-devel] [RFC PATCH v1 03/22] target/i386: Use prefix, aflag and dflag from DisasContext |
Date: |
Fri, 2 Aug 2019 09:20:16 -0400 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.8.0 |
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 <address@hidden
> <mailto:address@hidden>> wrote:
>
>
>
> On Wed, Jul 31, 2019 at 7:59 PM Jan Bobek <address@hidden
> <mailto:address@hidden>> wrote:
>
> From: Richard Henderson <address@hidden <mailto:address@hidden>>
>
> 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 <address@hidden
> <mailto:address@hidden>>
> ---
> 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
>
signature.asc
Description: OpenPGP digital signature
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: [Qemu-devel] [RFC PATCH v1 03/22] target/i386: Use prefix, aflag and dflag from DisasContext,
Jan Bobek <=