Removing typename from A_Const (was: Empty arrays with ARRAY[])

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On Fri, Mar 21, 2008 at 7:47 AM, Tom Lane wrote:
> I didn't do anything about removing A_Const's typename field, but I'm
> thinking that would be a good cleanup patch.
>

Here's my attempt to remove the typename field from A_Const. There
were a few places (notably flatten_set_variable_args() in guc.c, and
typenameTypeMod() in parse_type.c) where the code expected to see an
A_Const with a typename, and I had to adjust for an A_Const within a
TypeCast. Nonetheless, there was an overall net reduction of 34 lines
of code, so I think this was a win.

All regression tests passed on x86_64 gentoo.

Added to May CommitFest.

Cheers,
BJ
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.7 (GNU/Linux)
Comment: http://getfiregpg.org

iD8DBQFIA8s/5YBsbHkuyV0RAlMdAJ0dWdoZd5cypvInAR2msO8XA8qqxACeILSw
bCI2TGAQI3m3TBoJspvV4OQ=
=dGP9
-----END PGP SIGNATURE-----

Brendan Jurd escribió:

> Here's my attempt to remove the typename field from A_Const. There
> were a few places (notably flatten_set_variable_args() in guc.c, and
> typenameTypeMod() in parse_type.c) where the code expected to see an
> A_Const with a typename, and I had to adjust for an A_Const within a
> TypeCast. Nonetheless, there was an overall net reduction of 34 lines
> of code, so I think this was a win.

Do say ... why don't we do away with A_Const altogether and just replace
it with Value? After this patch, I don't see what's the difference.

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> Brendan Jurd escribi:
>> Here's my attempt to remove the typename field from A_Const. There
>> were a few places (notably flatten_set_variable_args() in guc.c, and
>> typenameTypeMod() in parse_type.c) where the code expected to see an
>> A_Const with a typename, and I had to adjust for an A_Const within a
>> TypeCast. Nonetheless, there was an overall net reduction of 34 lines
>> of code, so I think this was a win.

> Do say ... why don't we do away with A_Const altogether and just replace
> it with Value? After this patch, I don't see what's the difference.

They're logically different things, and after I get done putting a parse
location field into A_Const, they'll still be physically different too.

regards, tom lane

Tom Lane escribió:
> Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> > Brendan Jurd escribi:
> >> Here's my attempt to remove the typename field from A_Const. There
> >> were a few places (notably flatten_set_variable_args() in guc.c, and
> >> typenameTypeMod() in parse_type.c) where the code expected to see an
> >> A_Const with a typename, and I had to adjust for an A_Const within a
> >> TypeCast. Nonetheless, there was an overall net reduction of 34 lines
> >> of code, so I think this was a win.
>
> > Do say ... why don't we do away with A_Const altogether and just replace
> > it with Value? After this patch, I don't see what's the difference.
>
> They're logically different things, and after I get done putting a parse
> location field into A_Const, they'll still be physically different too.

Aha. Are you working from Brendan's patch? I was going to commit it.

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> Tom Lane escribi:
>> They're logically different things, and after I get done putting a parse
>> location field into A_Const, they'll still be physically different too.

> Aha. Are you working from Brendan's patch? I was going to commit it.

Sure, go ahead. I was going to add the parse location at the same time,
but it can perfectly well be done as a separate patch.

regards, tom lane

Tom Lane escribió:
> Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> > Tom Lane escribió:
> >> They're logically different things, and after I get done putting a parse
> >> location field into A_Const, they'll still be physically different too.
>
> > Aha. Are you working from Brendan's patch? I was going to commit it.
>
> Sure, go ahead. I was going to add the parse location at the same time,
> but it can perfectly well be done as a separate patch.

I came up with the attached patch. I added the location bits (although
I am unsure if I got the locations right in the parser), but they are
unused -- figuring out how to use them would take me longer than I can
to spend on this.

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera escribió:
> Tom Lane escribió:
> > Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> > > Tom Lane escribió:
> > >> They're logically different things, and after I get done putting a parse
> > >> location field into A_Const, they'll still be physically different too.
> >
> > > Aha. Are you working from Brendan's patch? I was going to commit it.
> >
> > Sure, go ahead. I was going to add the parse location at the same time,
> > but it can perfectly well be done as a separate patch.
>
> I came up with the attached patch. I added the location bits (although
> I am unsure if I got the locations right in the parser), but they are
> unused -- figuring out how to use them would take me longer than I can
> to spend on this.

Hmm, I'm now wondering if the location should be added to Value as well,
so that it can be passed down to Const?

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> I came up with the attached patch.

I wasn't envisioning anything anywhere near this invasive. We only
need locations on constants in a few contexts, I think.

BTW, you broke _equalAConst() ... it was a bad idea anyway to recast
it on the assumption that there would never again be more than one
field.

regards, tom lane

Tom Lane escribió:
> Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> > I came up with the attached patch.
>
> I wasn't envisioning anything anywhere near this invasive. We only
> need locations on constants in a few contexts, I think.

Aha. OK, I'll commit the original patch and let you deal with the rest
of it :-)

> BTW, you broke _equalAConst() ... it was a bad idea anyway to recast
> it on the assumption that there would never again be more than one
> field.

Oops, sorry, I had intended to revert that part and forgot.

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> Hmm, I'm now wondering if the location should be added to Value as well,
> so that it can be passed down to Const?

Just for the record, we don't want it in Const. Parse locations are
only useful in the "raw grammar" output, mainly because they aren't
helpful unless you still have the original query string laying about.

regards, tom lane