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

[Lluís Vilanova]

Lluís Vilanova writes:

> 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).

Aha, just checked your proposed patches more closely and it totally makes sense
to keep "is_jmp" to simplify the diffs, so I'll go for that one.

Thanks!
Lluis