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