Re: [Qemu-devel] [PATCH v6 3/6] target: [tcg] Add generic translation framework

[Lluís Vilanova]

Lluís Vilanova writes:

> Emilio G Cota writes:
>> On Thu, Jun 15, 2017 at 18:19:11 -0400, Emilio G. Cota wrote:
>>> (snip)
>>> > +/**
>>> > + * DisasContextBase:
>>> > + * @tb: Translation block for this disassembly.
>>> > + * @pc_first: Address of first guest instruction in this TB.
>>> > + * @pc_next: Address of next guest instruction in this TB (current during
>>> > + *           disassembly).
>>> > + * @num_insns: Number of translated instructions (including current).
>>> > + * @singlestep_enabled: "Hardware" single stepping enabled.
>>> > + *
>>> > + * Architecture-agnostic disassembly context.
>>> > + */
>>> > +typedef struct DisasContextBase {
>>> > +    TranslationBlock *tb;
>>> > +    target_ulong pc_first;
>>> > +    target_ulong pc_next;
>>> > +    DisasJumpType jmp_type;
>>> > +    unsigned int num_insns;
>>> > +    bool singlestep_enabled;
>>> > +} DisasContextBase;
>>> 
>>> - @pc_next: I'd stick with @pc, it's shorter, it's everywhere already, and
>>> with the documentation it's very clear what it is for.
>>> - @jmp_type: missing doc :-)

>> Also, consider keeping the @is_jmp name instead of renaming it to
>> @jmp_type. (@jmp would be shorter but it would be confusing though,
>> e.g. cris has both dc->jmp and dc->is_jmp.)

> I just figured that this series could also take the chance of trying to 
> rename a
> few common variables I'm changing to something more readable.

> But if you feel very strongly about keeping the original names (and minimizing
> the diffs as you say later), I'll revert the name changes.

Also, going through the changes to break them down into smaller pieces, I saw
that TranslationBlock (at least in i386) already has a "pc" member, so using
"pc_next" in DisasContextBase makes it even clearer it's a different variable.

You comments still apply to "is_jmp" vs "jmp_type" though. Unless you or anybody
else feels strongly against it, I'll keep "jmp_type", since I'm already changing
all lines that reference "is_jmp" to use DisasContextBase (instead of
DisasContext).


Thanks,
  Lluis