https://bugs.kde.org/show_bug.cgi?id=385408
--- Comment #11 from Vadim Barkov <vbr...@gmail.com> ---
(In reply to Julian Seward from comment #8)
> (In reply to Vadim Barkov from comment #6)
> * Divide it up into much smaller pieces, each of which can be reviewed
> independently.
I divided code in logical parts as you suggested. Hope the review process will
be MUCH easier now. Note that patch is supposed to be fully applied for compile
and regtest purposes.
> * Remove all whitespace changes. These just add noise and make the
> reviewing process more difficult.
Done.
> * Remove the numerous __inline__ annotations in guest_s390_toIR.c.
> None of these functions are performance critical, and the compiler
> can decide for itself what to inline.
I've removed __inline__ from my helper functions but saved them for
vr_xxy_offset(...), put_vr_xxy(...) and get_vr_xxy() ones.
The reason to do so that their gpr "brothers" are declared with inline. I don't
know if it is important or not but want to declare similar functions with
similar annotations.
If you disagree with this statement I will remove __inline__ from all my helper
functions in "guest_s390_toIR.c" file.
Let me know your opinion on it.
> * Try to reduce the size of the patch as much as possible. A few
> small changes widely spaced is ideal. Right now, we have stuff
> like this
>
> -static void
> -s390_format_RRF_UUFR(const HChar *(*irgen)(UChar m3, UChar m4, UChar r1,
> - UChar r2),
> - UChar m3, UChar m4, UChar r1, UChar r2)
> +/* Write byte #6 of a vr to the guest state. */
> +static __inline__ void
> +put_vr_b6(UInt archreg, IRExpr *expr)
> {
> - const HChar *mnm = irgen(m3, m4, r1, r2);
> + vassert(typeOfIRExpr(irsb->tyenv, expr) == Ity_I8);
>
> - if (UNLIKELY(vex_traceflags & VEX_TRACE_FE))
> - s390_disasm(ENC5(MNM, FPR, UINT, GPR, UINT), mnm, r1, m3, r2, m4);
> + stmt(IRStmt_Put(vr_b6_offset(archreg), expr));
> }
>
> which is incomprehensible. Is put_vr_b6 a new function? A replacement
> for s390_format_RRF_UUFR? A modified version of s390_format_RRF_UUFR?
> It's impossible to tell.
Fixed.
--
You are receiving this mail because:
You are watching all bug changes.
