Updated patch attached.
On Sat, 2013-11-09 at 12:09 +0530, Amit Khandekar wrote:
> > > 2) I found the following check a bit confusing, maybe you can make
> it
> > > better
> > > if (!argmodes || argmodes[i] == PROARGMODE_IN || argmodes[i] ==
> > > PROARGMODE_INOUT || argmodes[i] == PROARGMODE_VARIADIC)
> >
> > Factored that out into a separate helper function.
>
>
> The statement needs to be split into 80 columns width lines.
done
> > > 3) Function level comment for pg_get_function_arg_default() is
> > > missing.
> >
> > I think the purpose of the function is clear.
>
> Unless a reader goes through the definition, it is not obvious whether
> the second function argument represents the parameter position in
> input parameters or it is the parameter position in *all* the function
> parameters (i.e. proargtypes or proallargtypes). I think at least a
> one-liner description of what this function does should be present.
done
> >
> > > 4) You can also add comments inside the function, for example the
> > > comment for the line:
> > > nth = inputargn - 1 - (proc->pronargs - proc->pronargdefaults);
> >
> > Suggestion?
>
> Yes, it is difficult to understand what's the logic behind this
> calculation, until we go read the documentation related to
> pg_proc.proargdefaults. I think this should be sufficient:
> /*
> * proargdefaults elements always correspond to the last N input
> arguments,
> * where N = pronargdefaults. So calculate the nth_default accordingly.
> */
done
> There should be an initial check to see if nth_arg is passed a
> negative value. It is used as an index into the argmodes array, so a
> -ve array index can cause a crash. Because
> pg_get_function_arg_default() can now be called by a user also, we
> need to validate the input values. I am ok with returning with an
> error "Invalid argument".
done
> ---
> Instead of :
> deparse_expression_pretty(node, NIL, false, false, 0, 0)
> you can use :
> deparse_expression(node, NIL, false, false)
>
done
