On Fri, 10 Mar 2023 at 04:36, Peter Smith <smithpb2250@gmail.com> wrote:
>
> Hi,
>
> During a recent code review, I was confused multiple times by the
> toptxn member of ReorderBufferTXN, which is defined only for
> sub-transactions.
>
> e.g. txn->toptxn member == NULL means the txn is a top level txn.
> e.g. txn->toptxn member != NULL means the txn is not a top level txn
>
> It makes sense if you squint and read it slowly enough, but IMO it's
> too easy to accidentally misinterpret the meaning when reading code
> that uses this member.
>
> ~
>
> Such code can be made easier to read just by introducing some simple macros:
>
> #define isa_toptxn(rbtxn) (rbtxn->toptxn == NULL)
> #define isa_subtxn(rbtxn) (rbtxn->toptxn != NULL)
> #define get_toptxn(rbtxn) (isa_subtxn(rbtxn) ? rbtxn->toptxn : rbtxn)
>
> ~
>
> PSA a small patch that does this.
>
> (Tests OK using make check-world)
>
> Thoughts?
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)
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;
Regards,
Vignesh
