Re: Issues for named/mixed function notation patch

On Thu, 2009-10-01 at 17:56 +1000, Brendan Jurd wrote:
> I've had a look through the documentation and cleaned up a few things.

Thanks, that is helpful. I have included that along with some other
comment updates in the attached patch.

Paval,

In ProcedureCreate(), you match the argument names to see if anything
changed (in which case you raise an error). The code for that looks more
complex than I expected because it keeps track of the two argument lists
using different array indexes (i and j). Is there a reason it you can't
just iterate through with something like:

  if (p_oldargmodes[i] == PROARGMODE_OUT ||
      p_oldargmodes[i] == PROARGMODE_TABLE)
    continue;

  if (strcmp(p_oldargnames[i], p_names[i]) != 0)
    elog(ERROR, ...

I'm oversimplifying, of course, but I don't understand why you need both
i and j. Also, there is some unnecessarily verbose code like:

  if (p_modes == NULL || (p_modes != NULL ...

In func_get_detail(), there is:

  /* shouldn't happen, FuncnameGetCandidates messed up */
  if (best_candidate->ndargs > pform->pronargdefaults)
    elog(ERROR, "not enough default arguments");

Why is it only an error if ndargs is greater? Shouldn't they be equal?

  /*
  
   * Actually only first nargs field of best_candidate->args is valid,
   * Now, we have to refresh last ndargs items.
   */

Can you explain the above comment?

Please review Brendan and my patches and combine them with your patch.

Regards,
    Jeff Davis