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 > wrote: > > > > On Wed, Jul 31, 2019 at 7:59 PM Jan Bobek > wrote: > > From: Richard Henderson > > > 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 > > --- >  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 >