Re: Department of Redundancy Department: makeNode(FuncCall) division

* David Fetter (david(at)fetter(dot)org) wrote:
> On Mon, Feb 11, 2013 at 10:48:38AM -0800, David Fetter wrote:
> > On Sun, Feb 10, 2013 at 10:09:19AM -0500, Tom Lane wrote:
> > > David Fetter <david(at)fetter(dot)org> writes:
> > > > Per suggestions and lots of help from Andrew Gierth, please find
> > > > attached a patch to clean up the call sites for FuncCall nodes, which
> > > > I'd like to expand centrally rather than in each of the 37 (or 38, but
> > > > I only redid 37) places where it's called. The remaining one is in
> > > > src/backend/nodes/copyfuncs.c, which has to be modified for any
> > > > changes in the that struct anyhow.
> > >
> > > TBH, I don't think this is an improvement.
> > >
> > > The problem with adding a new field to any struct is that you have to
> > > run around and examine (and, usually, modify) every place that
> > > manufactures that type of struct. With a makeFuncCall defined like
> > > this, you'd still have to do that; it would just become a lot easier
> > > to forget to do so.

I don't really see how finding all callers of makeFuncCall is
particularly harder than finding the callers of makeNode(Func). If
there were cases where we still wanted to use makeNode(Func), perhaps
that would be annoying since you'd have to look for both- but, iiuc,
this patch changes all of the callers to use makeFuncCall and it seems
reasonable for all callers to do so in the future as well.

It looks to me like the advantage of this patch is exactly that you
*don't* have to run around and modify things which are completely
unrelated to the new feature being added (eg: FILTER). Add the new
field, set up the default/no-op case in makeFuncCall() and then only
change those places that actually need to worry about your new field.

> > > If the subroutine were defined like most other makeXXX subroutines,
> > > ie you have to supply *all* the fields, that argument would go away,
> > > but the notational advantage is then dubious.

Having to supply all the fields certainly wouldn't make things any
better. Providing the base set of fields which are required to be set
for any FuncCall node does make sense though, which is what the patch
does. The rest of the fields are all special cases for which a default
value works perfectly fine (when the field isn't involved in the
specific case being handled).

> > > The bigger-picture point is that you're proposing to make the coding
> > > conventions for building FuncCalls different from what they are for
> > > any other grammar node. I don't think that's a great idea; it will
> > > mostly foster confusion.
> >
> > The major difference between FuncCalls and others is that `while most
> > raw-parsetree nodes are constructed only in their own syntax
> > productions, FuncCall is constructed in many places unrelated to
> > actual function call syntax.

Yeah, FuncCall's are already rather special and they're built all over
the place.

That's my 2c on it anyhow. I don't see it as some kind of major
milestone but it looks like improvement to me and likely to make things
a bit easier on patch authors and reviewers who otherwise have to ponder
a bunch of repeated 'x->q = false;' statements in areas which are
completely unrelated to the new feature itself.

Thanks,

Stephen