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

On Tue, Jun 25, 2013 at 11:11 AM, Jeevan Chalke <
jeevan(dot)chalke(at)enterprisedb(dot)com> wrote:

> Hi David,
>
> I hope this is the latest patch to review, right ?
>
> I am going to review it.
>
> I have gone through the discussion on this thread and I agree with Stephen
> Frost that it don't add much improvements as such but definitely it is
> going to be easy for contributors in this area as they don't need to go all
> over to add any extra parameter they need to add. This patch simplifies it
> well enough.
>
> Will post my review soon.
>
>
Assuming *makeFuncArgs_002.diff* is the latest patch, here are my review
points.

About this patch and feature:
===
This patch tries to reduce redundancy when we need FuncCall expression. With
this patch it will become easier to add new parameter if needed. We just
need
to put it's default value at centralized location (I mean in this new
function)
so that all other places need not require and changes. And this new
parameter
is handled by the new feature who demanded it keep other untouched.

Review comments on patch:
===
1. Can't apply with "git apply" command but able to do it with patch -p1.
So no
issues
2. Adds one whitespace error, hopefully it will get corrected once it goes
through pgindent.
3. There is absolutely NO user visibility and thus I guess we don't need any
test case. Also no documentation are needed.
4. Also make check went smooth and thus assumes that there is no issue as
such.
Even I couldn't find any issue with my testing other than regression suite.
5. I had a code walk-through over patch and it looks good to me. However,
following line change seems unrelated (Line 186 in your patch)

! $$ = (Node *) makeSimpleA_Expr(AEXPR_OP, "~~", $1,
(Node *) n, @2);
!

Changes required from author:
===
It will be good if you remove unrelated changes from the patch and possibly
all
white-space errors.

Thanks

> Thanks.
>
> On Mon, Jun 17, 2013 at 11:13 AM, 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.
>> > >
>> > > 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.
>> > >
>> > > 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.
>> >
>> > This really will make things a good bit easier on our successors.
>>
>> Here's a rebased patch with comments illustrating how best to employ.
>>
>> In my previous message, I characterized the difference between
>> FuncCalls and other raw-parsetree nodes. Is there some flaw in that
>> logic? If there isn't, is there some reason not to treat them in a
>> less redundant, more uniform manner as this patch does?
>>
>> Cheers,
>> David.
>> --
>> David Fetter <david(at)fetter(dot)org> http://fetter.org/
>> Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
>> Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
>> iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics
>>
>> Remember to vote!
>> Consider donating to Postgres: http://www.postgresql.org/about/donate
>>
>>
>> --
>> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
>> To make changes to your subscription:
>> http://www.postgresql.org/mailpref/pgsql-hackers
>>
>>
>
>
> --
> Jeevan B Chalke
> Senior Software Engineer, R&D
> EnterpriseDB Corporation
> The Enterprise PostgreSQL Company
>
> Phone: +91 20 30589500
>
> Website: www.enterprisedb.com
> EnterpriseDB Blog: http://blogs.enterprisedb.com/
> Follow us on Twitter: http://www.twitter.com/enterprisedb
>
> This e-mail message (and any attachment) is intended for the use of the
> individual or entity to whom it is addressed. This message contains
> information from EnterpriseDB Corporation that may be privileged,
> confidential, or exempt from disclosure under applicable law. If you are
> not the intended recipient or authorized to receive this for the intended
> recipient, any use, dissemination, distribution, retention, archiving, or
> copying of this communication is strictly prohibited. If you have received
> this e-mail in error, please notify the sender immediately by reply e-mail
> and delete this message.
>

--
Jeevan B Chalke
Senior Software Engineer, R&D
EnterpriseDB Corporation
The Enterprise PostgreSQL Company

Phone: +91 20 30589500

Website: www.enterprisedb.com
EnterpriseDB Blog: http://blogs.enterprisedb.com/
Follow us on Twitter: http://www.twitter.com/enterprisedb

This e-mail message (and any attachment) is intended for the use of the
individual or entity to whom it is addressed. This message contains
information from EnterpriseDB Corporation that may be privileged,
confidential, or exempt from disclosure under applicable law. If you are
not the intended recipient or authorized to receive this for the intended
recipient, any use, dissemination, distribution, retention, archiving, or
copying of this communication is strictly prohibited. If you have received
this e-mail in error, please notify the sender immediately by reply e-mail
and delete this message.