Re: [Qemu-devel] [RFC PATCH v3 02/46] target/i386: Push rex_w into DisasContext

[Aleksandar Markovic]

15.08.2019. 11.55, "Richard Henderson" <address@hidden> је
написао/ла:
>
> On 8/15/19 8:30 AM, Aleksandar Markovic wrote:
> >
> > 15.08.2019. 04.13, "Jan Bobek" <address@hidden
> > <mailto:address@hidden>> је написао/ла:
> >>
> >> From: Richard Henderson <address@hidden <mailto:address@hidden>>
> >>
> >> Treat this the same as we already do for other rex bits.
> >>
> >> Signed-off-by: Richard Henderson <address@hidden <mailto:
address@hidden>>
> >> ---
> >>  target/i386/translate.c | 19 +++++++++++--------
> >>  1 file changed, 11 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/target/i386/translate.c b/target/i386/translate.c
> >> index d74dbfd585..c0866c2797 100644
> >> --- a/target/i386/translate.c
> >> +++ b/target/i386/translate.c
> >> @@ -44,11 +44,13 @@
> >>  #define REX_X(s) ((s)->rex_x)
> >>  #define REX_B(s) ((s)->rex_b)
> >>  #define REX_R(s) ((s)->rex_r)
> >> +#define REX_W(s) ((s)->rex_w)
> >>  #else
> >>  #define CODE64(s) 0
> >>  #define REX_X(s) 0
> >>  #define REX_B(s) 0
> >>  #define REX_R(s) 0
> >> +#define REX_W(s) -1
> >
> > The commit message says "treat rex_w the same as other rex bits". Why
is then
> > REX_W() treated differently here?
>
> "Treated the same" in terms of being referenced by a macro instead of a
local
> variable.  As for the -1, if you look at the rest of the patch you can
clearly
> see it preserves existing behaviour.
>

That is exactly what I dislike about your commit messages: they often
introduce ambiguity, without any real need, and with really bad
consequences to the reader. Is adding "in terms of being referenced by a
macro instead of a local
variable" to the commit message that hard?

When writing commit messages, you need to try to put yourself in the shoes
of the reader.

Aleksandar

> >> @@ -4503,6 +4504,7 @@ static target_ulong disas_insn(DisasContext *s,
> > CPUState *cpu)
> >>      s->rex_x = 0;
> >>      s->rex_b = 0;
> >>      s->rex_r = 0;
> >> +    s->rex_w = -1;
> >>      s->x86_64_hregs = false;
> >>  #endif
> >>      s->rip_offset = 0; /* for relative ip address */
> >> @@ -4514,7 +4516,6 @@ static target_ulong disas_insn(DisasContext *s,
> > CPUState *cpu)
> >>      }
> >>
> >>      prefixes = 0;
> >> -    rex_w = -1;
>
>
> r~