So I'm looking to commit this, and here's my comments so far:
WindowClauseSortNode - I don't like this name, because it's not actually
a Node of any kind. How about WindowSortData?
list_concat_unique(list_copy(x),y) is exactly list_union(x,y), which
looks a bit nicer to me.
re. this:
for (; nActive > 0; nActive--)
result = lcons(actives[nActive - 1].wc, result);
Now that we're allowed to use C99, I think it looks better like this:
for (int i = 0; i < nActive; i++)
result = lappend(result, actives[i].wc);
(Building lists in forward order by using a reversed construction and
iterating backwards seems like an unnecessary double-negative.)
I can add a comment about not needing to compare eqop (which is derived
directly from sortop, so it can't differ unless sortop also does -
provided sortop is actually present; if window partitions could be
hashed, this would be a problem, but that doesn't strike me as very
likely to happen).
Any comments? (no need to post further patches unless there's some major
change needed)
--
Andrew (irc:RhodiumToad)
