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.chalke@enterprisedb.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 for the review!
Please find attached the latest patch.
Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.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