|From:||David Fetter <david(at)fetter(dot)org>|
|To:||Jeevan Chalke <jeevan(dot)chalke(at)enterprisedb(dot)com>|
|Cc:||Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, PG Hackers <pgsql-hackers(at)postgresql(dot)org>|
|Subject:||Re: Department of Redundancy Department: makeNode(FuncCall) division|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
On Tue, Jun 25, 2013 at 02:54:55PM +0530, Jeevan Chalke wrote:
> 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
> 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
> to put it's default value at centralized location (I mean in this new
> so that all other places need not require and changes. And this new
> 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
> 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
> 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
> white-space errors.
Thanks for the review!
Please find attached the latest patch.
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
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
|Next Message||Peter Eisentraut||2013-06-28 14:26:14||Re: changeset generation v5-01 - Patches & git tree|
|Previous Message||Robert Haas||2013-06-28 13:49:53||Re: extensible external toast tuple support|