Re: [GENERAL] Empty arrays with ARRAY[]

On Nov 28, 2007 4:19 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> "Brendan Jurd" <direvus(at)gmail(dot)com> writes:
> > Now I'm thinking I leave the grammar rules alone (apart from making it
> > legal to specify an empty list of elements), and instead push the
> > typename down into the child node from makeTypeCast(), if the child is
> > an A_ArrayExpr. Does that work better?
>
> Actually, if you do that you might as well forego the separate node type
> (which requires a nontrivial amount of infrastructure). I think it
> would work just about as well to have transformExpr check whether the
> argument of a TypeCast is an ArrayExpr, and if so call
> transformArrayExpr directly from there, passing the TypeName as an
> additional argument.

I actually thought that A_ArrayExpr would be a good addition even if
you ignore the matter of typecasting. It always seemed weird to me
that the parser generates an ArrayExpr directly. ArrayExpr has a
bunch of members that are only set by the transform; all the parser
does is set the 'elements' member. And then the transform creates a
brand new ArrayExpr and populates it based on what's in the 'elements'
member of the otherwise-empty ArrayExpr passed to it.

So my feeling is that an A_ArrayExpr is a better fit for the parser
output than ArrayExpr, and more in keeping with how the rest of the
code does things.

Mind you I'm also okay with your suggestion to let transformExpr take
care of it. But I'm not adverse to putting in the legwork to set up
the infrastructure for A_ArrayExpr, if it's a nice outcome.

> Kinda ugly, but not really any worse than the way
> A_Const is handled in that same routine. (In fact, we could use the
> same technique to get rid of the typename field in A_Const ... might
> be worth doing?)

I had a bit of a dig into this. A_Const->typename gets set directly
by the parse paths for "INTERVAL [(int)] string [interval range]". In
fact, as far as I can tell that's the _only_ place A_Const->typename
gets used at all. And all the transform does with that piece of
information is treat the node like a typecast.

I'm not seeing a huge amount of value in this special treatment. Why
not just have the parser build this as an A_Const inside a TypeCast
and then let the transform deal with it in the usual way? I found the
following comment at parsenodes.h:244

* NOTE: for mostly historical reasons, A_Const parsenodes contain
* room for a TypeName; we only generate a separate TypeCast node if the
* argument to be casted is not a constant. In theory either representation
* would work, but the combined representation saves a bit of code in many
* productions in gram.y.

However, this is no longer the case. makeTypeCast() doesn't care
about whether its argument is a constant anymore:

* Earlier we would determine whether an A_Const would
* be acceptable, however Domains require coerce_type()
* to process them -- applying constraints as required.

And in "many productions in gram.y", "many" == 2. Currently the
combined representation requires more code than it saves.

So, I get the impression the use-case for A_Const->typename has become
extinct. I think it could be removed with a minimum of fuss, and I'd
be happy to include same with my patch (or, submit it as a separate
patch; let me know your preference).

Regards,
BJ