Thanks for the review! On Mon, Mar 13, 2023 at 6:19 PM vignesh C <vignes...@gmail.com> wrote: ... > Few comments: > 1) Can we move the macros along with the other macros present in this > file, just above this structure, similar to the macros added for > txn_flags: > /* Toplevel transaction for this subxact (NULL for top-level). */ > +#define isa_toptxn(rbtxn) (rbtxn->toptxn == NULL) > +#define isa_subtxn(rbtxn) (rbtxn->toptxn != NULL) > +#define get_toptxn(rbtxn) (isa_subtxn(rbtxn) ? rbtxn->toptxn : rbtxn) > > 2) The macro name can be changed to rbtxn_is_toptxn, rbtxn_is_subtxn > and rbtxn_get_toptxn to keep it consistent with others: > /* Toplevel transaction for this subxact (NULL for top-level). */ > +#define isa_toptxn(rbtxn) (rbtxn->toptxn == NULL) > +#define isa_subtxn(rbtxn) (rbtxn->toptxn != NULL) > +#define get_toptxn(rbtxn) (isa_subtxn(rbtxn) ? rbtxn->toptxn : rbtxn) > > 3) We could add separate comments for each of the macros: > /* Toplevel transaction for this subxact (NULL for top-level). */ > +#define isa_toptxn(rbtxn) (rbtxn->toptxn == NULL) > +#define isa_subtxn(rbtxn) (rbtxn->toptxn != NULL) > +#define get_toptxn(rbtxn) (isa_subtxn(rbtxn) ? rbtxn->toptxn : rbtxn) >
All the above are fixed as suggested. > 4) We check if txn->toptxn is not null twice here both in if condition > and in the assignment, we could retain the assignment operation as > earlier to remove the 2nd check: > - if (txn->toptxn) > - txn = txn->toptxn; > + if (isa_subtxn(txn)) > + txn = get_toptxn(txn); > > We could avoid one check again by: > + if (isa_subtxn(txn)) > + txn = txn->toptxn; > Yeah, that is true, but I chose not to keep the original assignment in this case mainly because then it is consistent with the other changed code --- e.g. Every other direct member assignment/access of the 'toptxn' member now hides behind the macros so I did not want this single place to be the odd one out. TBH, I don't think 1 extra check is of any significance, but it is not a problem to change like you suggested if other people also want it done that way. ------ Kind Regards, Peter Smith. Fujitsu Australia.
v2-0001-Add-macros-for-ReorderBufferTXN-toptxn.patch
Description: Binary data
