Andres Freund <[email protected]> writes:
> On 2019-07-31 19:40:09 -0400, Tom Lane wrote:
>> That makes the other idea (of a foreach-ish macro declaring the
>> listcell value variable) problematic, too :-(.
> Hm. One way partially around that would be using an anonymous struct
> inside the for(). Something like
> #define foreach_node(membertype, name, lst) \
> for (struct {membertype *node; ListCell *lc; const List *l; int i;} name =
> {...}; \
> ...)
> which then would allow code like
> foreach_node(OpExpr, cur, list)
> {
> do_something_with_node(cur.node);
> foreach_delete_current(cur);
> }
I'm hesitant to change the look of our loops quite that much, mainly
because it'll be a pain for back-patching. If you write some code
for HEAD like this, and then have to back-patch it, you'll need to
insert/change significantly more code than if it's just a matter
of whether there's a ListCell variable or not.
I experimented with the "aforeach" idea I suggested upthread,
to the extent of writing the macros and then converting
parse_clause.c (a file chosen more or less at random) to use
aforeach instead of foreach. I was somewhat surprised to find
that every single foreach() did convert pleasantly. (There are
several forboth's that I didn't try to do anything with, though.)
If we do go in this direction, I wouldn't suggest trying to
actually do wholesale conversion of existing code like this;
that seems more likely to create back-patching land mines than
do anything helpful. I am slightly tempted to try to convert
everyplace using foreach_delete_current, though, since those
loops are different from v12 already.
Thoughts?
regards, tom lane
diff --git a/src/backend/parser/parse_clause.c b/src/backend/parser/parse_clause.c
index 2a6b2ff..39d8d8e 100644
--- a/src/backend/parser/parse_clause.c
+++ b/src/backend/parser/parse_clause.c
@@ -117,8 +117,6 @@ static Node *transformFrameOffset(ParseState *pstate, int frameOptions,
void
transformFromClause(ParseState *pstate, List *frmList)
{
- ListCell *fl;
-
/*
* The grammar will have produced a list of RangeVars, RangeSubselects,
* RangeFunctions, and/or JoinExprs. Transform each one (possibly adding
@@ -128,9 +126,9 @@ transformFromClause(ParseState *pstate, List *frmList)
* Note we must process the items left-to-right for proper handling of
* LATERAL references.
*/
- foreach(fl, frmList)
+ aforeach(frmList)
{
- Node *n = lfirst(fl);
+ Node *n = (Node *) aforeach_current();
RangeTblEntry *rte;
int rtindex;
List *namespace;
@@ -267,11 +265,10 @@ extractRemainingColumns(List *common_colnames,
{
char *colname = strVal(lfirst(lnames));
bool match = false;
- ListCell *cnames;
- foreach(cnames, common_colnames)
+ aforeach(common_colnames)
{
- char *ccolname = strVal(lfirst(cnames));
+ char *ccolname = strVal(aforeach_current());
if (strcmp(colname, ccolname) == 0)
{
@@ -475,7 +472,6 @@ transformRangeFunction(ParseState *pstate, RangeFunction *r)
List *coldeflists = NIL;
bool is_lateral;
RangeTblEntry *rte;
- ListCell *lc;
/*
* We make lateral_only names of this level visible, whether or not the
@@ -505,9 +501,9 @@ transformRangeFunction(ParseState *pstate, RangeFunction *r)
* Likewise, collect column definition lists if there were any. But
* complain if we find one here and the RangeFunction has one too.
*/
- foreach(lc, r->functions)
+ aforeach(r->functions)
{
- List *pair = (List *) lfirst(lc);
+ List *pair = aforeach_current_node(List);
Node *fexpr;
List *coldeflist;
Node *newfexpr;
@@ -551,11 +547,9 @@ transformRangeFunction(ParseState *pstate, RangeFunction *r)
fc->over == NULL &&
coldeflist == NIL)
{
- ListCell *lc;
-
- foreach(lc, fc->args)
+ aforeach(fc->args)
{
- Node *arg = (Node *) lfirst(lc);
+ Node *arg = (Node *) aforeach_current();
FuncCall *newfc;
last_srf = pstate->p_last_srf;
@@ -700,7 +694,6 @@ transformRangeTableFunc(ParseState *pstate, RangeTableFunc *rtf)
Oid docType;
RangeTblEntry *rte;
bool is_lateral;
- ListCell *col;
char **names;
int colno;
@@ -743,9 +736,9 @@ transformRangeTableFunc(ParseState *pstate, RangeTableFunc *rtf)
names = palloc(sizeof(char *) * list_length(rtf->columns));
colno = 0;
- foreach(col, rtf->columns)
+ aforeach(rtf->columns)
{
- RangeTableFuncCol *rawc = (RangeTableFuncCol *) lfirst(col);
+ RangeTableFuncCol *rawc = aforeach_current_node(RangeTableFuncCol);
Oid typid;
int32 typmod;
Node *colexpr;
@@ -837,15 +830,13 @@ transformRangeTableFunc(ParseState *pstate, RangeTableFunc *rtf)
/* Namespaces, if any, also need to be transformed */
if (rtf->namespaces != NIL)
{
- ListCell *ns;
- ListCell *lc2;
List *ns_uris = NIL;
List *ns_names = NIL;
bool default_ns_seen = false;
- foreach(ns, rtf->namespaces)
+ aforeach(rtf->namespaces)
{
- ResTarget *r = (ResTarget *) lfirst(ns);
+ ResTarget *r = aforeach_current_node(ResTarget);
Node *ns_uri;
Assert(IsA(r, ResTarget));
@@ -858,9 +849,9 @@ transformRangeTableFunc(ParseState *pstate, RangeTableFunc *rtf)
/* Verify consistency of name list: no dupes, only one DEFAULT */
if (r->name != NULL)
{
- foreach(lc2, ns_names)
+ aforeach(ns_names)
{
- Value *ns_node = (Value *) lfirst(lc2);
+ Value *ns_node = (Value *) aforeach_current();
if (ns_node == NULL)
continue;
@@ -1268,19 +1259,17 @@ transformFromClauseItem(ParseState *pstate, Node *n,
if (j->isNatural)
{
List *rlist = NIL;
- ListCell *lx,
- *rx;
Assert(j->usingClause == NIL); /* shouldn't have USING() too */
- foreach(lx, l_colnames)
+ aforeach(l_colnames)
{
- char *l_colname = strVal(lfirst(lx));
+ char *l_colname = strVal(aforeach_current());
Value *m_name = NULL;
- foreach(rx, r_colnames)
+ aforeach(r_colnames)
{
- char *r_colname = strVal(lfirst(rx));
+ char *r_colname = strVal(aforeach_current());
if (strcmp(l_colname, r_colname) == 0)
{
@@ -1313,24 +1302,21 @@ transformFromClauseItem(ParseState *pstate, Node *n,
List *ucols = j->usingClause;
List *l_usingvars = NIL;
List *r_usingvars = NIL;
- ListCell *ucol;
Assert(j->quals == NULL); /* shouldn't have ON() too */
- foreach(ucol, ucols)
+ aforeach(ucols)
{
- char *u_colname = strVal(lfirst(ucol));
- ListCell *col;
- int ndx;
+ char *u_colname = strVal(aforeach_current());
int l_index = -1;
int r_index = -1;
Var *l_colvar,
*r_colvar;
/* Check for USING(foo,foo) */
- foreach(col, res_colnames)
+ aforeach(res_colnames)
{
- char *res_colname = strVal(lfirst(col));
+ char *res_colname = strVal(aforeach_current());
if (strcmp(res_colname, u_colname) == 0)
ereport(ERROR,
@@ -1340,10 +1326,9 @@ transformFromClauseItem(ParseState *pstate, Node *n,
}
/* Find it in left input */
- ndx = 0;
- foreach(col, l_colnames)
+ aforeach(l_colnames)
{
- char *l_colname = strVal(lfirst(col));
+ char *l_colname = strVal(aforeach_current());
if (strcmp(l_colname, u_colname) == 0)
{
@@ -1352,9 +1337,8 @@ transformFromClauseItem(ParseState *pstate, Node *n,
(errcode(ERRCODE_AMBIGUOUS_COLUMN),
errmsg("common column name \"%s\" appears more than once in left table",
u_colname)));
- l_index = ndx;
+ l_index = aforeach_current_index();
}
- ndx++;
}
if (l_index < 0)
ereport(ERROR,
@@ -1363,10 +1347,9 @@ transformFromClauseItem(ParseState *pstate, Node *n,
u_colname)));
/* Find it in right input */
- ndx = 0;
- foreach(col, r_colnames)
+ aforeach(r_colnames)
{
- char *r_colname = strVal(lfirst(col));
+ char *r_colname = strVal(aforeach_current());
if (strcmp(r_colname, u_colname) == 0)
{
@@ -1375,9 +1358,8 @@ transformFromClauseItem(ParseState *pstate, Node *n,
(errcode(ERRCODE_AMBIGUOUS_COLUMN),
errmsg("common column name \"%s\" appears more than once in right table",
u_colname)));
- r_index = ndx;
+ r_index = aforeach_current_index();
}
- ndx++;
}
if (r_index < 0)
ereport(ERROR,
@@ -1390,7 +1372,7 @@ transformFromClauseItem(ParseState *pstate, Node *n,
r_colvar = list_nth(r_colvars, r_index);
r_usingvars = lappend(r_usingvars, r_colvar);
- res_colnames = lappend(res_colnames, lfirst(ucol));
+ res_colnames = lappend(res_colnames, aforeach_current());
res_colvars = lappend(res_colvars,
buildMergedJoinVar(pstate,
j->jointype,
@@ -1643,11 +1625,9 @@ makeNamespaceItem(RangeTblEntry *rte, bool rel_visible, bool cols_visible,
static void
setNamespaceColumnVisibility(List *namespace, bool cols_visible)
{
- ListCell *lc;
-
- foreach(lc, namespace)
+ aforeach(namespace)
{
- ParseNamespaceItem *nsitem = (ParseNamespaceItem *) lfirst(lc);
+ ParseNamespaceItem *nsitem = (ParseNamespaceItem *) aforeach_current();
nsitem->p_cols_visible = cols_visible;
}
@@ -1660,11 +1640,9 @@ setNamespaceColumnVisibility(List *namespace, bool cols_visible)
static void
setNamespaceLateralState(List *namespace, bool lateral_only, bool lateral_ok)
{
- ListCell *lc;
-
- foreach(lc, namespace)
+ aforeach(namespace)
{
- ParseNamespaceItem *nsitem = (ParseNamespaceItem *) lfirst(lc);
+ ParseNamespaceItem *nsitem = (ParseNamespaceItem *) aforeach_current();
nsitem->p_lateral_only = lateral_only;
nsitem->p_lateral_ok = lateral_ok;
@@ -1822,8 +1800,6 @@ static TargetEntry *
findTargetlistEntrySQL92(ParseState *pstate, Node *node, List **tlist,
ParseExprKind exprKind)
{
- ListCell *tl;
-
/*----------
* Handle two special cases as mandated by the SQL92 spec:
*
@@ -1895,9 +1871,9 @@ findTargetlistEntrySQL92(ParseState *pstate, Node *node, List **tlist,
{
TargetEntry *target_result = NULL;
- foreach(tl, *tlist)
+ aforeach(*tlist)
{
- TargetEntry *tle = (TargetEntry *) lfirst(tl);
+ TargetEntry *tle = aforeach_current_node(TargetEntry);
if (!tle->resjunk &&
strcmp(tle->resname, name) == 0)
@@ -1944,9 +1920,9 @@ findTargetlistEntrySQL92(ParseState *pstate, Node *node, List **tlist,
parser_errposition(pstate, location)));
target_pos = intVal(val);
- foreach(tl, *tlist)
+ aforeach(*tlist)
{
- TargetEntry *tle = (TargetEntry *) lfirst(tl);
+ TargetEntry *tle = aforeach_current_node(TargetEntry);
if (!tle->resjunk)
{
@@ -1990,7 +1966,6 @@ findTargetlistEntrySQL99(ParseState *pstate, Node *node, List **tlist,
ParseExprKind exprKind)
{
TargetEntry *target_result;
- ListCell *tl;
Node *expr;
/*
@@ -2002,9 +1977,9 @@ findTargetlistEntrySQL99(ParseState *pstate, Node *node, List **tlist,
*/
expr = transformExpr(pstate, node, exprKind);
- foreach(tl, *tlist)
+ aforeach(*tlist)
{
- TargetEntry *tle = (TargetEntry *) lfirst(tl);
+ TargetEntry *tle = aforeach_current_node(TargetEntry);
Node *texpr;
/*
@@ -2094,7 +2069,6 @@ flatten_grouping_sets(Node *expr, bool toplevel, bool *hasGroupingSets)
case T_GroupingSet:
{
GroupingSet *gset = (GroupingSet *) expr;
- ListCell *l2;
List *result_set = NIL;
if (hasGroupingSets)
@@ -2109,9 +2083,9 @@ flatten_grouping_sets(Node *expr, bool toplevel, bool *hasGroupingSets)
if (toplevel && gset->kind == GROUPING_SET_EMPTY)
return (Node *) NIL;
- foreach(l2, gset->content)
+ aforeach(gset->content)
{
- Node *n1 = lfirst(l2);
+ Node *n1 = (Node *) aforeach_current();
Node *n2 = flatten_grouping_sets(n1, false, NULL);
if (IsA(n1, GroupingSet) &&
@@ -2139,13 +2113,13 @@ flatten_grouping_sets(Node *expr, bool toplevel, bool *hasGroupingSets)
case T_List:
{
List *result = NIL;
- ListCell *l;
- foreach(l, (List *) expr)
+ aforeach((List *) expr)
{
- Node *n = flatten_grouping_sets(lfirst(l), toplevel, hasGroupingSets);
+ Node *n = (Node *) aforeach_current();
- if (n != (Node *) NIL)
+ n = flatten_grouping_sets(n, toplevel, hasGroupingSets);
+ if (n)
{
if (IsA(n, List))
result = list_concat(result, (List *) n);
@@ -2200,8 +2174,6 @@ transformGroupClauseExpr(List **flatresult, Bitmapset *seen_local,
if (tle->ressortgroupref > 0)
{
- ListCell *sl;
-
/*
* Eliminate duplicates (GROUP BY x, x) but only at local level.
* (Duplicates in grouping sets can affect the number of returned
@@ -2239,10 +2211,9 @@ transformGroupClauseExpr(List **flatresult, Bitmapset *seen_local,
* another sort step is going to be inevitable, but that's the
* planner's problem.
*/
-
- foreach(sl, sortClause)
+ aforeach(sortClause)
{
- SortGroupClause *sc = (SortGroupClause *) lfirst(sl);
+ SortGroupClause *sc = aforeach_current_node(SortGroupClause);
if (sc->tleSortGroupRef == tle->ressortgroupref)
{
@@ -2298,11 +2269,10 @@ transformGroupClauseList(List **flatresult,
{
Bitmapset *seen_local = NULL;
List *result = NIL;
- ListCell *gl;
- foreach(gl, list)
+ aforeach(list)
{
- Node *gexpr = (Node *) lfirst(gl);
+ Node *gexpr = (Node *) aforeach_current();
Index ref = transformGroupClauseExpr(flatresult,
seen_local,
@@ -2349,14 +2319,13 @@ transformGroupingSet(List **flatresult,
List **targetlist, List *sortClause,
ParseExprKind exprKind, bool useSQL99, bool toplevel)
{
- ListCell *gl;
List *content = NIL;
Assert(toplevel || gset->kind != GROUPING_SET_SETS);
- foreach(gl, gset->content)
+ aforeach(gset->content)
{
- Node *n = lfirst(gl);
+ Node *n = (Node *) aforeach_current();
if (IsA(n, List))
{
@@ -2371,7 +2340,7 @@ transformGroupingSet(List **flatresult,
}
else if (IsA(n, GroupingSet))
{
- GroupingSet *gset2 = (GroupingSet *) lfirst(gl);
+ GroupingSet *gset2 = (GroupingSet *) n;
content = lappend(content, transformGroupingSet(flatresult,
pstate, gset2,
@@ -2455,7 +2424,6 @@ transformGroupClause(ParseState *pstate, List *grouplist, List **groupingSets,
List *result = NIL;
List *flat_grouplist;
List *gsets = NIL;
- ListCell *gl;
bool hasGroupingSets = false;
Bitmapset *seen_local = NULL;
@@ -2481,9 +2449,9 @@ transformGroupClause(ParseState *pstate, List *grouplist, List **groupingSets,
exprLocation((Node *) grouplist)));
}
- foreach(gl, flat_grouplist)
+ aforeach(flat_grouplist)
{
- Node *gexpr = (Node *) lfirst(gl);
+ Node *gexpr = (Node *) aforeach_current();
if (IsA(gexpr, GroupingSet))
{
@@ -2555,11 +2523,10 @@ transformSortClause(ParseState *pstate,
bool useSQL99)
{
List *sortlist = NIL;
- ListCell *olitem;
- foreach(olitem, orderlist)
+ aforeach(orderlist)
{
- SortBy *sortby = (SortBy *) lfirst(olitem);
+ SortBy *sortby = aforeach_current_node(SortBy);
TargetEntry *tle;
if (useSQL99)
@@ -2587,11 +2554,10 @@ transformWindowDefinitions(ParseState *pstate,
{
List *result = NIL;
Index winref = 0;
- ListCell *lc;
- foreach(lc, windowdefs)
+ aforeach(windowdefs)
{
- WindowDef *windef = (WindowDef *) lfirst(lc);
+ WindowDef *windef = aforeach_current_node(WindowDef);
WindowClause *refwc = NULL;
List *partitionClause;
List *orderClause;
@@ -2805,8 +2771,6 @@ transformDistinctClause(ParseState *pstate,
List **targetlist, List *sortClause, bool is_agg)
{
List *result = NIL;
- ListCell *slitem;
- ListCell *tlitem;
/*
* The distinctClause should consist of all ORDER BY items followed by all
@@ -2823,9 +2787,9 @@ transformDistinctClause(ParseState *pstate,
* effect will be that the TLE value will be made unique according to both
* sortops.
*/
- foreach(slitem, sortClause)
+ aforeach(sortClause)
{
- SortGroupClause *scl = (SortGroupClause *) lfirst(slitem);
+ SortGroupClause *scl = aforeach_current_node(SortGroupClause);
TargetEntry *tle = get_sortgroupclause_tle(scl, *targetlist);
if (tle->resjunk)
@@ -2843,9 +2807,9 @@ transformDistinctClause(ParseState *pstate,
* Now add any remaining non-resjunk tlist items, using default sort/group
* semantics for their data types.
*/
- foreach(tlitem, *targetlist)
+ aforeach(*targetlist)
{
- TargetEntry *tle = (TargetEntry *) lfirst(tlitem);
+ TargetEntry *tle = aforeach_current_node(TargetEntry);
if (tle->resjunk)
continue; /* ignore junk */
@@ -2902,9 +2866,9 @@ transformDistinctOnClause(ParseState *pstate, List *distinctlist,
* Also notice that we could have duplicate DISTINCT ON expressions and
* hence duplicate entries in sortgrouprefs.)
*/
- foreach(lc, distinctlist)
+ aforeach(distinctlist)
{
- Node *dexpr = (Node *) lfirst(lc);
+ Node *dexpr = (Node *) aforeach_current();
int sortgroupref;
TargetEntry *tle;
@@ -2923,9 +2887,9 @@ transformDistinctOnClause(ParseState *pstate, List *distinctlist,
* in DISTINCT ON.
*/
skipped_sortitem = false;
- foreach(lc, sortClause)
+ aforeach(sortClause)
{
- SortGroupClause *scl = (SortGroupClause *) lfirst(lc);
+ SortGroupClause *scl = aforeach_current_node(SortGroupClause);
if (list_member_int(sortgrouprefs, scl->tleSortGroupRef))
{
@@ -3021,11 +2985,10 @@ resolve_unique_index_expr(ParseState *pstate, InferClause *infer,
Relation heapRel)
{
List *result = NIL;
- ListCell *l;
- foreach(l, infer->indexElems)
+ aforeach(infer->indexElems)
{
- IndexElem *ielem = (IndexElem *) lfirst(l);
+ IndexElem *ielem = aforeach_current_node(IndexElem);
InferenceElem *pInfer = makeNode(InferenceElem);
Node *parse;
@@ -3422,16 +3385,15 @@ Index
assignSortGroupRef(TargetEntry *tle, List *tlist)
{
Index maxRef;
- ListCell *l;
if (tle->ressortgroupref) /* already has one? */
return tle->ressortgroupref;
/* easiest way to pick an unused refnumber: max used + 1 */
maxRef = 0;
- foreach(l, tlist)
+ aforeach(tlist)
{
- Index ref = ((TargetEntry *) lfirst(l))->ressortgroupref;
+ Index ref = aforeach_current_node(TargetEntry)->ressortgroupref;
if (ref > maxRef)
maxRef = ref;
@@ -3463,15 +3425,14 @@ bool
targetIsInSortList(TargetEntry *tle, Oid sortop, List *sortList)
{
Index ref = tle->ressortgroupref;
- ListCell *l;
/* no need to scan list if tle has no marker */
if (ref == 0)
return false;
- foreach(l, sortList)
+ aforeach(sortList)
{
- SortGroupClause *scl = (SortGroupClause *) lfirst(l);
+ SortGroupClause *scl = aforeach_current_node(SortGroupClause);
if (scl->tleSortGroupRef == ref &&
(sortop == InvalidOid ||
@@ -3489,11 +3450,9 @@ targetIsInSortList(TargetEntry *tle, Oid sortop, List *sortList)
static WindowClause *
findWindowClause(List *wclist, const char *name)
{
- ListCell *l;
-
- foreach(l, wclist)
+ aforeach(wclist)
{
- WindowClause *wc = (WindowClause *) lfirst(l);
+ WindowClause *wc = aforeach_current_node(WindowClause);
if (wc->name && strcmp(wc->name, name) == 0)
return wc;
diff --git a/src/include/nodes/pg_list.h b/src/include/nodes/pg_list.h
index 1463408..21d0a67 100644
--- a/src/include/nodes/pg_list.h
+++ b/src/include/nodes/pg_list.h
@@ -381,6 +381,59 @@ lnext(const List *l, const ListCell *c)
#define foreach_current_index(cell) (cell##__state.i)
/*
+ * aforeach (anonymous foreach) -
+ * another convenience macro for looping through a list
+ *
+ * This is the same as foreach() except that no ListCell variable is needed.
+ * Instead, reference the current list element with the appropriate variant
+ * of aforeach_current().
+ */
+#define aforeach(lst) \
+ for (ForEachState aforeach__state = {(lst), 0}; \
+ aforeach__state.l != NIL && \
+ aforeach__state.i < aforeach__state.l->length; \
+ aforeach__state.i++)
+
+/*
+ * aforeach_current... -
+ *
+ * Access the current element of the most closely nested aforeach() loop.
+ * These exist in the same variants that lfirst...() has.
+ */
+#define aforeach_current() \
+ (aforeach__state.l->elements[aforeach__state.i].ptr_value)
+#define aforeach_current_int() \
+ (aforeach__state.l->elements[aforeach__state.i].int_value)
+#define aforeach_current_oid() \
+ (aforeach__state.l->elements[aforeach__state.i].oid_value)
+#define aforeach_current_node(type) \
+ castNode(type, aforeach_current())
+
+/*
+ * aforeach_delete_current -
+ * delete the current list element from the List associated with the most
+ * closely nested aforeach() loop, returning the new List pointer.
+ *
+ * This is equivalent to list_delete_nth_cell(), but it also adjusts the
+ * aforeach loop's state so that no list elements will be missed. Do not
+ * delete elements from an active aforeach loop's list in any other way!
+ */
+#define aforeach_delete_current() \
+ ((List *) (aforeach__state.l = list_delete_nth_cell(aforeach__state.l, \
+ aforeach__state.i--)))
+
+/*
+ * aforeach_current_index -
+ * get the zero-based list index of the most closely nested aforeach()
+ * loop's current element.
+ *
+ * Beware of using this after aforeach_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 aforeach_current_index() (aforeach__state.i)
+
+/*
* for_each_cell -
* a convenience macro which loops through a list starting from a
* specified cell
