From 14dd0f27d7cd56ffae9ecdbe324965073d01a9ff Mon Sep 17 00:00:00 2001 From: Nathan Bossart Date: Thu, 4 Jan 2024 16:09:34 -0600 Subject: [PATCH] Add macros for looping through a List without a ListCell. Many foreach loops only use the ListCell pointer to retrieve the content of the cell, like so: ListCell *lc; foreach(lc, mylist) { int myint = lfirst_int(lc); ... } This commit adds a few convenience macros that automatically declare the loop variable and retrieve the current cell's contents. This allows us to rewrite the previous loop like this: foreach_int(myint, mylist) { ... } This commit also adjusts a few existing loops in order to add coverage for the new/adjusted macros. There is presently no plan to bulk update all foreach loops, as that could introduce a significant amount of back-patching pain. Instead, these macros are primarily intended for use in new code. Author: Jelte Fennema-Nio Reviewed-by: David Rowley, Alvaro Herrera, Vignesh C, Tom Lane Discussion: https://postgr.es/m/CAGECzQSwXKnxGwW1_Q5JE%2B8Ja20kyAbhBHO04vVrQsLcDciwXA%40mail.gmail.com --- src/backend/executor/execExpr.c | 9 +-- src/backend/replication/logical/relation.c | 4 +- src/backend/replication/logical/tablesync.c | 6 +- src/backend/replication/pgoutput/pgoutput.c | 7 +- src/include/nodes/pg_list.h | 71 ++++++++++++++++++--- 5 files changed, 71 insertions(+), 26 deletions(-) diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c index cd41522b6b2..91df2009bee 100644 --- a/src/backend/executor/execExpr.c +++ b/src/backend/executor/execExpr.c @@ -216,7 +216,6 @@ ExecInitQual(List *qual, PlanState *parent) ExprState *state; ExprEvalStep scratch = {0}; List *adjust_jumps = NIL; - ListCell *lc; /* short-circuit (here and in ExecQual) for empty restriction list */ if (qual == NIL) @@ -250,10 +249,8 @@ ExecInitQual(List *qual, PlanState *parent) scratch.resvalue = &state->resvalue; scratch.resnull = &state->resnull; - foreach(lc, qual) + foreach_ptr(Expr, node, qual) { - Expr *node = (Expr *) lfirst(lc); - /* first evaluate expression */ ExecInitExprRec(node, state, &state->resvalue, &state->resnull); @@ -265,9 +262,9 @@ ExecInitQual(List *qual, PlanState *parent) } /* adjust jump targets */ - foreach(lc, adjust_jumps) + foreach_int(jump, adjust_jumps) { - ExprEvalStep *as = &state->steps[lfirst_int(lc)]; + ExprEvalStep *as = &state->steps[jump]; Assert(as->opcode == EEOP_QUAL); Assert(as->d.qualexpr.jumpdone == -1); diff --git a/src/backend/replication/logical/relation.c b/src/backend/replication/logical/relation.c index 136e2532578..c68e8cfab7a 100644 --- a/src/backend/replication/logical/relation.c +++ b/src/backend/replication/logical/relation.c @@ -746,11 +746,9 @@ static Oid FindUsableIndexForReplicaIdentityFull(Relation localrel, AttrMap *attrmap) { List *idxlist = RelationGetIndexList(localrel); - ListCell *lc; - foreach(lc, idxlist) + foreach_oid(idxoid, idxlist) { - Oid idxoid = lfirst_oid(lc); bool isUsableIdx; Relation idxRel; IndexInfo *idxInfo; diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c index e8cc9ac5522..06d5b3df33a 100644 --- a/src/backend/replication/logical/tablesync.c +++ b/src/backend/replication/logical/tablesync.c @@ -1036,11 +1036,11 @@ fetch_remote_table_info(char *nspname, char *relname, /* Build the pubname list. */ initStringInfo(&pub_names); - foreach(lc, MySubscription->publications) + foreach_node(String, pubstr, MySubscription->publications) { - char *pubname = strVal(lfirst(lc)); + char *pubname = strVal(pubstr); - if (foreach_current_index(lc) > 0) + if (foreach_current_index(pubstr) > 0) appendStringInfoString(&pub_names, ", "); appendStringInfoString(&pub_names, quote_literal_cstr(pubname)); diff --git a/src/backend/replication/pgoutput/pgoutput.c b/src/backend/replication/pgoutput/pgoutput.c index 1d0ed5c36f7..425238187f6 100644 --- a/src/backend/replication/pgoutput/pgoutput.c +++ b/src/backend/replication/pgoutput/pgoutput.c @@ -2234,7 +2234,6 @@ cleanup_rel_sync_cache(TransactionId xid, bool is_commit) { HASH_SEQ_STATUS hash_seq; RelationSyncEntry *entry; - ListCell *lc; Assert(RelationSyncCache != NULL); @@ -2247,15 +2246,15 @@ cleanup_rel_sync_cache(TransactionId xid, bool is_commit) * corresponding schema and we don't need to send it unless there is * any invalidation for that relation. */ - foreach(lc, entry->streamed_txns) + foreach_xid(streamed_txn, entry->streamed_txns) { - if (xid == lfirst_xid(lc)) + if (xid == streamed_txn) { if (is_commit) entry->schema_sent = true; entry->streamed_txns = - foreach_delete_current(entry->streamed_txns, lc); + foreach_delete_current(entry->streamed_txns, streamed_txn); break; } } diff --git a/src/include/nodes/pg_list.h b/src/include/nodes/pg_list.h index fdc8f090310..52df93759f7 100644 --- a/src/include/nodes/pg_list.h +++ b/src/include/nodes/pg_list.h @@ -381,26 +381,26 @@ lnext(const List *l, const ListCell *c) /* * foreach_delete_current - * delete the current list element from the List associated with a - * surrounding foreach() loop, returning the new List pointer. + * surrounding foreach() or foreach_*() loop, returning the new List + * pointer; pass the name of the iterator variable. * - * This is equivalent to list_delete_cell(), but it also adjusts the foreach - * loop's state so that no list elements will be missed. Do not delete - * elements from an active foreach loop's list in any other way! + * This is similar to list_delete_cell(), but it also adjusts the loop's state + * so that no list elements will be missed. Do not delete elements from an + * active foreach or foreach_* loop's list in any other way! */ -#define foreach_delete_current(lst, cell) \ - (cell##__state.i--, \ - (List *) (cell##__state.l = list_delete_cell(lst, cell))) +#define foreach_delete_current(lst, var_or_cell) \ + ((List *) (var_or_cell##__state.l = list_delete_nth_cell(lst, var_or_cell##__state.i--))) /* * foreach_current_index - - * get the zero-based list index of a surrounding foreach() loop's - * current element; pass the name of the "ListCell *" iterator variable. + * get the zero-based list index of a surrounding foreach() or foreach_*() + * loop's current element; pass the name of the iterator variable. * * Beware of using this after foreach_delete_current(); the value will be * out of sync for the rest of the current loop iteration. Anyway, since * you just deleted the current element, the value is pretty meaningless. */ -#define foreach_current_index(cell) (cell##__state.i) +#define foreach_current_index(var_or_cell) (var_or_cell##__state.i) /* * for_each_from - @@ -452,6 +452,57 @@ for_each_cell_setup(const List *lst, const ListCell *initcell) return r; } +/* + * Convenience macros that loop through a list without needing a separate + * "ListCell *" variable. Instead, the macros declare a locally-scoped loop + * variable with the provided name and the appropriate type. + * + * Since the variable is scoped to the loop, it's not possible to detect an + * early break by checking its value after the loop completes, as is common + * practice. If you need to do this, you can either use foreach() instead or + * manually track early breaks with a separate variable declared outside of the + * loop. + * + * Note that the caveats described in the comment above the foreach() macro + * also apply to these convenience macros. + */ +#define foreach_ptr(type, var, lst) foreach_internal(type, *, var, lst, lfirst) +#define foreach_int(var, lst) foreach_internal(int, , var, lst, lfirst_int) +#define foreach_oid(var, lst) foreach_internal(Oid, , var, lst, lfirst_oid) +#define foreach_xid(var, lst) foreach_internal(TransactionId, , var, lst, lfirst_xid) + +/* + * The internal implementation of the above macros. Do not use directly. + * + * This macro actually generates two loops in order to declare two variables of + * different types. The outer loop only iterates once, so we expect optimizing + * compilers will unroll it, thereby optimizing it away. + */ +#define foreach_internal(type, pointer, var, lst, func) \ + for (type pointer var = 0, pointer var##__outerloop = (type pointer) 1; \ + var##__outerloop; \ + var##__outerloop = 0) \ + for (ForEachState var##__state = {(lst), 0}; \ + (var##__state.l != NIL && \ + var##__state.i < var##__state.l->length && \ + (var = func(&var##__state.l->elements[var##__state.i]), true)); \ + var##__state.i++) + +/* + * foreach_node - + * The same as foreach_ptr, but asserts that the element is of the specified + * node type. + */ +#define foreach_node(type, var, lst) \ + for (type * var = 0, *var##__outerloop = (type *) 1; \ + var##__outerloop; \ + var##__outerloop = 0) \ + for (ForEachState var##__state = {(lst), 0}; \ + (var##__state.l != NIL && \ + var##__state.i < var##__state.l->length && \ + (var = lfirst_node(type, &var##__state.l->elements[var##__state.i]), true)); \ + var##__state.i++) + /* * forboth - * a convenience macro for advancing through two linked lists