Hi Maciej, Philippe -- thank you for your reviews,
On Mon, Sep 17, 2018 at 06:10:27PM +0100, Maciej W. Rozycki wrote: > Nitpicking here, but I think it's what makes code clean and pleasant to > read. I agree, that is important too. I will post an updated v5 soon. Another alternative change is to define check_insn_opc_user_only as static inline void check_insn_opc_user_only(DisasContext *ctx, int flags) { #ifndef CONFIG_USER_ONLY check_insn_opc_removed(ctx, flags); #endif } by referring to check_insn_opc_removed (instead of copying its definition). Would you consider this an improvement for v5 too? > I think it would make sense to keep the order of the checks consistent, > or otherwise code starts looking messy. > > The predominant order appears to be (as applicable) starting with a > check for the lowest ISA membership, followed by a check for the highest > ISA membership (R6 instruction cuts) and ending with processor-specific > special cases. I think this order makes sense in that it starts with > checks that have a wider scope and then moves on to ones of a narrower > scope, and I think keeping it would be good (as would fixing where the > addition of R6 broke it). Mode checks for otherwise existing > instructions then follow, which complements the pattern. Sure, thanks for clarifying the ordering rules! > So please make it: > > check_insn(ctx, ISA_MIPS3); > check_insn_opc_user_only(ctx, INSN_R5900); > check_mips_64(ctx); Done. > > @@ -25275,6 +25280,7 @@ static void decode_opc(CPUMIPSState *env, > > DisasContext *ctx) > > break; > > case OPC_SCD: > > check_insn_opc_removed(ctx, ISA_MIPS32R6); > > + check_insn_opc_user_only(ctx, INSN_R5900); > > check_insn(ctx, ISA_MIPS3); > > check_mips_64(ctx); > > gen_st_cond(ctx, op, rt, rs, imm); > > And please make it: > > check_insn_opc_removed(ctx, ISA_MIPS32R6); > check_insn(ctx, ISA_MIPS3); > check_insn_opc_user_only(ctx, INSN_R5900); > check_mips_64(ctx); Done. > here (and swapping the two former calls ought to be fixed separately; I > haven't checked if there are more cases like this, but if so, then they > would best be amended with a single change). I'll defer other ordering and indentation fixes since I'm not sure whether such changes would be accepted. Fredrik